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

fix(mappers): Schema passthrough for whitelisted fields #1219

Conversation

edgarrmondragon
Copy link
Collaborator

@edgarrmondragon edgarrmondragon commented Nov 24, 2022

Related:

We'll have to release a new version of the SDK, and bump it in MeltanoLabs/meltano-map-transform.


I changed the test_mapped_stream to use snapshots so we can validate the entire tap output.


📚 Documentation preview 📚: https://meltano-sdk--1219.org.readthedocs.build/en/1219/

@codecov
Copy link

codecov bot commented Nov 24, 2022

Codecov Report

Merging #1219 (61469de) into main (417e75a) will increase coverage by 0.04%.
The diff coverage is 100.00%.

❗ Current head 61469de differs from pull request most recent head f43369f. Consider uploading reports for the commit f43369f to get more accurate results

@@            Coverage Diff             @@
##             main    #1219      +/-   ##
==========================================
+ Coverage   83.68%   83.72%   +0.04%     
==========================================
  Files          42       42              
  Lines        3910     3909       -1     
  Branches      666      665       -1     
==========================================
+ Hits         3272     3273       +1     
  Misses        473      473              
+ Partials      165      163       -2     
Impacted Files Coverage Δ
singer_sdk/helpers/_typing.py 52.79% <100.00%> (+0.32%) ⬆️
singer_sdk/streams/core.py 85.74% <0.00%> (+0.24%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@edgarrmondragon edgarrmondragon force-pushed the 66-mapper-assumes-wrong-schema-when-just-whitelisting-fields branch from edbdb95 to b34f509 Compare November 28, 2022 16:33
@edgarrmondragon edgarrmondragon marked this pull request as ready for review November 28, 2022 16:37
@aaronsteers
Copy link
Contributor

aaronsteers commented Nov 28, 2022

@edgarrmondragon - This is great.

I like this approach a lot, but the library is new to me and I'm not sure if I would know where to look to confirm the changes. So, if we fix the double-state messages at the end of each sync (for example), I expect the job would fail. And I think (?) then the proper course for developer would then be to run a snapshot update, then confirm the git diff of those snapshots that got updated, and if the new schema files are better or at least as good, then those could be merged back to main.

Could we add something in CONTRIBUTING.md about the snapshots feature and how they should be maintained/updated?

Thanks!

@edgarrmondragon
Copy link
Collaborator Author

Could we add something in CONTRIBUTING.md about the snapshots feature and how they should be maintained/updated?

@aaronsteers I've updated the contributing page with more details: af1ef39, https://meltano-sdk--1219.org.readthedocs.build/en/1219/CONTRIBUTING.html#snapshot-testing

@edgarrmondragon edgarrmondragon enabled auto-merge (squash) November 29, 2022 15:37
@edgarrmondragon edgarrmondragon merged commit d23ebbd into main Nov 29, 2022
@edgarrmondragon edgarrmondragon deleted the 66-mapper-assumes-wrong-schema-when-just-whitelisting-fields branch November 29, 2022 15:43
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.

3 participants