Skip to content

Re-name types in policy#937

Open
tcharding wants to merge 7 commits intorust-bitcoin:masterfrom
tcharding:push-kwnxuvkkrolm
Open

Re-name types in policy#937
tcharding wants to merge 7 commits intorust-bitcoin:masterfrom
tcharding:push-kwnxuvkkrolm

Conversation

@tcharding
Copy link
Copy Markdown
Member

As described here: #885 (comment)

Big diff but this is mostly re-names and code moves. I split it up in steps. First might be best to look at the final state of policy, especially I'm not sure if we want the re-exports left in there considering deprecation doesn't work.

Currently the `policy::Concrete` and `policy::Semantic` types are
alias'. We would like to refer to them as `policy::Policy` and
`policy::semantic::Semantic` (according to a review suggestion). I
believe this is to make it more clear that one shouldn't need to use
the semantic policy?

Deprecation doesn't work for type alias', just use a code comment to
show what is going no.

Deprecate the old alias'. Re-export `concrete::Policy` by its own
name. Add an alias to `semantic`. Next we will remove the alias and
re-name the `semantic::Policy` type. Done separately to make it super
obvious what is going on.
To differentiate it from the concrete policy (I think).
    
After the re-name this stutters. Import type and remove the path.
Introduce a `pollicy::lift` module and move the lifting types and
logic there. Add deprecated re-exports although these may not
function correctly, I didn't check.
Now we have done re-names and the concrete policy is the 'first class'
type in `policy` inline the logic from the submodle to `policy/mod.rs`.
Just use `Policy` now we have done all the re-names.
@apoelstra
Copy link
Copy Markdown
Member

concept ACK these changes. But can you split this into two or three PRs:

  • one that renames semantic::Policy to semantic::Semantic (and reexports it from the crate root and removes all the stutters, if you want)
  • one that moves the Lift trait to the semantic module (can be combined with the above)
  • one that renames policy::Concrete to policy::Policy (and moves the code into policy/mod.rs, if you want) and deals with the fallout

Even if the deprecations don't work we should retain the aliases. Then people who are surprised by the breakage in a future version will at least have the ability to fix things by doing the renames then doing the upgrade.

@apoelstra
Copy link
Copy Markdown
Member

Oh -- to clarify the above. in this PR you move Lift into the policy module. It should be in the semantic module instead.

@tcharding
Copy link
Copy Markdown
Member Author

No worries. One question came to me. Isn't this PR going to wreak havoc on that big local branch you have? Is it worth doing now or should we just wait a bit since this is easy enough to do at any time?

@apoelstra
Copy link
Copy Markdown
Member

Heh, yeah, it will cause me some chaos, but I suspect I could jury-rig jj fix to deal with it for me. Or I can just fix the specific places where there are problems. For the most part I didn't make big changes to the policy module.

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.

2 participants