Introduce private modules and infrastructure#3844
Closed
tcharding wants to merge 3 commits intorust-bitcoin:masterfrom
Closed
Introduce private modules and infrastructure#3844tcharding wants to merge 3 commits intorust-bitcoin:masterfrom
tcharding wants to merge 3 commits intorust-bitcoin:masterfrom
Conversation
The readme is stale, we forgot to add the `API` job to the readme.
Closed
Member
Author
|
The idea for this PR was discussed in #3817 |
Member
Author
|
Ooph, I was hoping to minimze the size of the diff in this PR but it seems |
9dce036 to
ff32453
Compare
We currently share code by way of public code in the `internals` crate. This was a hack to reduce code duplication while trying to keep things private - it failed. We currently have at least one "private" type in the public API (`InputString`). In an effort to solve the original problem while fixing the mistake we came up with the idea of having a private directory in `internals` with a bunch of small modules in it. We define this as the Single Source Of Truth (SSOT). Then we copy this directory to other crates as needed. To ensure that we don't inadvertently update one and not the other we use CI to diff against the SSOT. Introduce the `internals/src/private` directory. Add a bunch of stuff to allow use to fully encapsulate the `InputString` type. Add infrastructure to CI to test that the new stuff in `private` stays in sync.
ff32453 to
b26b6e2
Compare
Member
Author
|
Reconsider after #3848 goes in. |
This was referenced Jan 4, 2025
Member
Author
|
Closing, we are going to try using the |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
EDIT: After sleeping on this I'm not happy with the removal of
int()change. I've had another idea.Note to reviewers:
Warning this is an f'ing nightmare. To review this I'd first look at the
contrib/diff-private.shscript. Then once you are convinced that all theprivatemodules are the same just look at the one ininternals.The
int()function proved too hard for me to solve so I inlined it. That change could be pulled out into a separate PR if needed.The original idea for this PR was discussed here: #3817 (comment)
We currently share code by way of public code in the
internalscrate. This was a hack to reduce code duplication while trying to keep things private - it failed. We currently have at least one "private" type in the public API (InputString).In an effort to solve the original problem while fixing the mistake we came up with the idea of having a private directory in
internalswith a bunch of small modules in it. We define this as the Single Source Of Truth (SSOT). Then we copy this directory to other crates as needed. To ensure that we don't inadvertently update one and not the other we use CI to diff against the SSOT.Introduce the
internals/src/privatedirectory. Add a bunch of stuff to allow use to fully encapsulate theInputStringtype.Add infrastructure to CI to test that the new stuff in
privatestays in sync.