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

Split test and bench out to separate packages #43

Merged
merged 1 commit into from
Feb 22, 2023

Conversation

wraithm
Copy link
Member

@wraithm wraithm commented Dec 1, 2022

#42

@wraithm wraithm marked this pull request as ready for review December 1, 2022 08:18
@@ -47,26 +44,6 @@ dependencies:
- vector >= 0.12.1.2
library:
source-dirs: src
other-modules: Bitcoin.Keys.Extended.Internal
Copy link
Member Author

Choose a reason for hiding this comment

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

This is maybe a questionable thing, but tbh, I really dislike other-modules as a thing. Like, what if I want the fingerprint's Word32 or whatever. Just lemme do it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I do not like representing Fingerprint as a Word32. How about we just change it to SizedByteArray 4 ByteString? Then I would be more comfortable exposing Fingerprint internals, and maybe getting rid of Bitcoin.Keys.Extended.Internal.

Copy link
Member Author

Choose a reason for hiding this comment

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

IMHO, that sounds out of scope for this changeset, but certainly an interesting thought.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agreed different changeset. I think exposing Internal modules is common practice in our ecosystem. People know the footguns they entail, let's just expose it.

benchmark/package.yaml Outdated Show resolved Hide resolved
@GambolingPangolin
Copy link
Contributor

The packaging is not very clear from the directory structure. How about reorganizing the code so that there are three folders, each of which contains a package?:

  • bitcoin
  • bitcoin-test
  • bitcoin-bench

In addition, we can keep README.md, stack.yaml, etc. at the top level.

@wraithm
Copy link
Member Author

wraithm commented Jan 15, 2023

The packaging is not very clear from the directory structure. How about reorganizing the code so that there are three folders, each of which contains a package?:

  • bitcoin
  • bitcoin-test
  • bitcoin-bench

In addition, we can keep README.md, stack.yaml, etc. at the top level.

I'm not opposed to that at all.

@ProofOfKeags, what do you think?

@ProofOfKeags
Copy link
Collaborator

100% onboard with that folder structure.

@ProofOfKeags
Copy link
Collaborator

After reviewing as well I'm fine to merge this after exposing internal modules and reorganizing the directories to be more intuitive.

@wraithm
Copy link
Member Author

wraithm commented Feb 13, 2023

@ProofOfKeags @GambolingPangolin This is ready for review.

@ProofOfKeags ProofOfKeags merged commit 8c6700d into master Feb 22, 2023
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

3 participants