Move and rename crate::policy::semantic::Policy to crate::r#abstract::Abstract#604
Closed
tcharding wants to merge 8 commits intorust-bitcoin:masterfrom
Closed
Move and rename crate::policy::semantic::Policy to crate::r#abstract::Abstract#604tcharding wants to merge 8 commits intorust-bitcoin:masterfrom
crate::policy::semantic::Policy to crate::r#abstract::Abstract#604tcharding wants to merge 8 commits intorust-bitcoin:masterfrom
Conversation
We can import types to test in unit tests using `use super::*`.
Member
Author
|
There will be some grammar mistakes in docs 'a' vs 'an' because of semantic to abstract change. Will fix later. |
tcharding
force-pushed
the
09-28-mv-policy
branch
from
16dc092 to
1b99d1d
Compare
We used "semantic" so as not to clash with the reserved Rust keyword "abstract", however there is a language feature to deal with such name clashes - the raw identifier syntax `r#foo`. Use `r#abstract` and rename the `policy/semantic.rs` file to `policy/abstract.rs`. There is more re-naming to do to complete the semantic -> abstract change but here we _only_ do the module name to ease review.
The `Liftable` trait is its own thing, it defines a trait to lift miniscript, descriptors, and concrete policies to an abstract policy. Move the `Liftable` and associated error type and impls to a new `lift` module that lives under the recently created `crate::r#abstract` module. Note we alse remove the `Semantic` and `Concrete` public re-names/re-exports from `crate::policy` because we are transitioning away from using the term "semantic", however to minimise the changes in this patch we add a bunch of `use crate::policy::concrete::Policy as Concrete` statements.
We are moving away from the usage of "semantic" as a synonym for "abstract". Use `Abstract` instead of `Semantic` as the type alias for `crate::policy::r#abstract::Policy`.
Make it explicit that the `Policy` returned by `lift` in the `descriptor::tr` module is a `r#abstract::Policy`.
The abstract policy is different from the concrete policy because it is only part of rust-miniscript and not miniscript in general. To help differentiate it move it to a separate module. Note to reviewers, `git` couldn't work out that this is basically a file move so I've left the `src/policy/abstract.rs` file in the repo but it is not included in the build. Reviewers can do `diff src/policy/abstract.rs src/abstract/mod.rs` to ease review. It will be removed in the next patch.
We do not build this file in the build, remove it.
Use the identifier `Abstract` to further disambiguate the abstract policy and the concrete policy. This has the nice advantage of removing a few usages of the kind-of-ugly `r#abstract::Policy`.
tcharding
force-pushed
the
09-28-mv-policy
branch
from
1b99d1d to
b79be87
Compare
Member
Author
|
TODO: Remove the serialization impl from |
Member
|
Sorry, but I think we should keep the module name as What if we called the module Or we could keep it as |
Member
Author
|
No sweat, I'll have another play with it. |
Member
Author
|
I did #607 so we can see what its like, I will need to close the unwanted one and [re]write the PR description. |
Member
Author
|
I don't think this is going anywhere |
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
2 participants
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.
Based on discussion in #603. This requires #603 to be updated.
Done as a series of steps, I don't usually do PRs like this but after reviewing the recent
bitcoin_ioPR I think it does help for making review easier when there are a load of simple but slightly different changes involved in getting from one state to another. Can squash, merge, or re-split if required. Please comment on the patch series style if you have any suggestions on how I can make review easier.Draft for feedback please, especially the last patch - the struct rename from
PolicytoAbstract. Note also this leavesconcrete::Policynamed as is.