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

Fixed Data.Avro.Schema.extractBindings. #62

Merged
merged 1 commit into from Oct 18, 2018

Conversation

TikhonJelvis
Copy link
Contributor

The old implementation had some odd bugs with nested type definitions, including missing types defined inside arrays and maps. This PR has a new implementation of extractBindings that fixes all the problems as well as a test case which covers a bunch of edge cases for extracting types from a schema.

The old implementation had some odd bugs with nested type definitions, including missing types defined inside arrays and maps. This PR has a new implementation of extractBindings that fixes all the problems as well as a test case which covers a bunch of edge cases for extracting types from a schema.
@TikhonJelvis
Copy link
Contributor Author

This addresses #61.

@TikhonJelvis
Copy link
Contributor Author

Assuming this implementation is actually correct—I believe it is—I recommend back-porting this to older versions of the package with bug fix releases (ie 0.3.5.2, 0.3.4.3 and so on).

@AlexeyRaga AlexeyRaga merged commit 00e471e into haskell-works:master Oct 18, 2018
@AlexeyRaga
Copy link
Member

Thanks!
The extractBindings functionality is fairly new. Do think it is a problem if we don't create bugfix releases, and instead assume that people can upgrade to the newest version?

@TikhonJelvis TikhonJelvis deleted the fix-extract-bindings branch October 18, 2018 09:24
@TikhonJelvis
Copy link
Contributor Author

It's not ideal. There have been some breaking changes recently, so upgrading isn't entirely trivial—I know it would require some extra updates/refactoring on one of my projects at work, for example. At the same time, the fix is pretty self-contained, so back-porting it should be straightforward.

@AlexeyRaga
Copy link
Member

OK, I'll try to find time to do it this weekend.

@AlexeyRaga
Copy link
Member

I looked at versions 0.3.4.x and 0.3.5.x and have found that extractBindings isn't a part of these versions. It has been first introduced in 0.3.6.0 so perhaps retro-fix isn't needed for previous versions.

@TikhonJelvis
Copy link
Contributor Author

@AlexeyRaga sorry, I didn't explain the full context here.

extractBindings was only extracted to a top-level function recently, but before that the same logic was part of buildTypeEnvironment. The older implementations of buildTypeEnvironment still have the same bug, at least with types defined inside maps. The retro fix may or may not define a top-level extractBindings, but it should fix the bug with buildTypeEnvironment.

By the way, I'm happy to write the code for fixing 0.3.4.x and 0.3.5.x, but I'm not sure how to organize it on GitHub. What can I open a PR against?

@AlexeyRaga
Copy link
Member

@TikhonJelvis I have created branches release-0.3.4, release-0.3.5 and release-0.3.6

If you make PRs into these branches then we'll be able to release patched versions for 0.3.4 and 0.3.5

@TikhonJelvis
Copy link
Contributor Author

@AlexeyRaga Awesome, thanks! I opened PRs against release-0.3.4 and release-0.3.5. The release-0.3.6 branch already has the correct behavior.

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.

None yet

2 participants