Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pe 6423 support pre 103 mobile #51

Merged

Conversation

ilias1111
Copy link
Collaborator

@ilias1111 ilias1111 commented Jun 10, 2024

Description

This PR fixes this bug, by using the databricks way of accessing a JSON, which as a result if the object is not present it will return null and not an error

What type of PR is this? (check all applicable)

  • 🍕 Feature
  • 🐛 Bug Fix
  • 📝 Documentation Update
  • 🎨 Style
  • 🧑‍💻 Code Refactor
  • 🔥 Performance Improvements
  • ✅ Test
  • 🤖 Build
  • 🔁 CI
  • 📦 Chore (Release)
  • ⏩ Revert

Related Tickets & Documents

Fixes #48

Checklist

  • 💣 Is your change a breaking change?
  • 📖 I have updated the CHANGELOG.md

Added tests?

  • 👍 yes
  • 🙅 no, because they aren't needed
  • 🙋 no, because I need help

Added to documentation?

  • 📓 internal package docs (ymls, macros, readme, if applicable)
  • 📕 I have raised a Snowplow documentation PR if applicable (Link here)
  • 🙅 no documentation needed

[optional] What gif best describes this PR or how it makes you feel?

Alt Text

@ilias1111 ilias1111 requested a review from a team as a code owner June 10, 2024 17:00
@ilias1111 ilias1111 changed the base branch from main to release/snowplow_unified/0.4.2 June 10, 2024 17:08
@rlh1994 rlh1994 force-pushed the release/snowplow_unified/0.4.2 branch from 4f395e2 to a8bd082 Compare June 10, 2024 19:32
@rlh1994
Copy link
Contributor

rlh1994 commented Jun 11, 2024

Can you rebase on the latest release branch please. Also - only if we actually have this in the integration tests I can't remember - could you remove that one of those fields from the integration tests source data (and update the downstream stuff as needed...) so that we can ensure it works as intended going forward please - if we don't have it in the tests then don't worry

@ilias1111 ilias1111 force-pushed the PE-6423-Support-pre-103-mobile branch from 2cae9cf to 1c1dcd0 Compare June 11, 2024 11:08
Copy link
Contributor

@rlh1994 rlh1994 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to be insanely picky, can we have all the type casting be consistantly cased please.

@ilias1111 ilias1111 requested a review from rlh1994 June 11, 2024 12:33
@ilias1111 ilias1111 marked this pull request as draft June 11, 2024 12:35
@ilias1111 ilias1111 marked this pull request as ready for review June 11, 2024 12:35
Copy link
Contributor

@rlh1994 rlh1994 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! don't forget to squash and merge with a suitable message (no need to reference the ticket in the message)

@ilias1111 ilias1111 merged commit 202e099 into release/snowplow_unified/0.4.2 Jun 11, 2024
5 checks passed
@rlh1994 rlh1994 deleted the PE-6423-Support-pre-103-mobile branch June 13, 2024 13:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Document native requirements
2 participants