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.

Sign up for GitHub

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

Jump to bottom

Feature/dependency pruning #27

Merged
merged 5 commits into from
Oct 10, 2022
Merged

Feature/dependency pruning #27

merged 5 commits into from
Oct 10, 2022

Conversation

ProofOfKeags
Copy link
Collaborator

Closes #17 and #19

@ProofOfKeags ProofOfKeags linked an issue Oct 4, 2022 that may be closed by this pull request
Remove mtl as a dependency #19
Closed
GambolingPangolin
GambolingPangolin reviewed Oct 5, 2022
View reviewed changes
Copy link
Contributor

@GambolingPangolin GambolingPangolin left a comment

Choose a reason for hiding this comment

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

The project does not build as is. The test suite depends on aeson as well.

@GambolingPangolin
Copy link
Contributor

Ok, the test suite actually should depend on aeson since there are a bunch of test vectors we parse out of json documents.

@ProofOfKeags
Copy link
Collaborator Author

ah yep. I need to add that to my workflow.

@ProofOfKeags
Copy link
Collaborator Author

ProofOfKeags commented Oct 5, 2022 •
edited
Loading

OK So this now checks out. Lotta work to maintain the json stuff. Does it actually buy us anything or can we move to make the test vectors into in-haskell values?

@ProofOfKeags
Copy link
Collaborator Author

This PR should be squashed on merge.

GambolingPangolin
GambolingPangolin reviewed Oct 9, 2022
View reviewed changes
Copy link
Contributor

@GambolingPangolin GambolingPangolin left a comment

Choose a reason for hiding this comment

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

Is there some way to simplify the process of parsing test vectors out of JSON encoded sources? Ideally the JSON code would be totally minimal and purpose built to deal with the test vectors. So we would not need e.g. NetBox and JsonBox.

test/Bitcoin/Orphans.hs Show resolved Hide resolved
src/Bitcoin/Util.hs Outdated Show resolved Hide resolved
test/Bitcoin/AddressSpec.hs Show resolved Hide resolved
test/Bitcoin/UtilSpec.hs Show resolved Hide resolved
@ProofOfKeags
Copy link
Collaborator Author

Is there some way to simplify the process of parsing test vectors out of JSON encoded sources? Ideally the JSON code would be totally minimal and purpose built to deal with the test vectors. So we would not need e.g. NetBox and JsonBox.

Probably. However, this change was primarily concerned with removing aeson and mtl from the main library code. I didn't really audit whether the code itself was "optimal". Personally I don't like the fact that we are giving typeclass instances to ToJSON and FromJSON to these types solely for the test vectors, as there is no guarantee that there is a canonical representation type here. I'd be fine with handrolling a Parser a for these types and using parseWith on the actual reading.

Keep in mind that a future PR will be moving from QuickCheck to hedgehog though so I'd rather not spend too much effort on this code in this particular patch. Happy to revamp the test code once the hedgehog patch is finished.

@GambolingPangolin
Copy link
Contributor

All right. I don't mind going back for a cleanup pass. The instances don't bother me much since they are confined to the test suite.

@ProofOfKeags
Copy link
Collaborator Author

Actually I can revamp this code during the hedgehog patch.

@ProofOfKeags ProofOfKeags merged commit c3c81d2 into master Oct 10, 2022
@GambolingPangolin GambolingPangolin deleted the feature/dependency-pruning branch December 4, 2022 20:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GambolingPangolin GambolingPangolin GambolingPangolin approved these changes

@wraithm wraithm Awaiting requested review from wraithm

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove mtl as a dependency Remove aeson as a dependency
2 participants
@ProofOfKeags @GambolingPangolin