Skip to content

Use Key instead of MiniscriptKey - #375

Closed
tcharding wants to merge 1 commit into
rust-bitcoin:masterfrom
tcharding:rename-miniscript-key
Closed

Use Key instead of MiniscriptKey#375
tcharding wants to merge 1 commit into
rust-bitcoin:masterfrom
tcharding:rename-miniscript-key

Conversation

@tcharding

Copy link
Copy Markdown
Member

One of the main traits in this lib is MiniscriptKey, we can shorten this to Key with no loss of meaning. This makes the whole codebase more terse. Terse code, if clear, is easier to read because there is less clutter. Also terseness assists the formatter and can reduce lines of code.

This patch is the result of running

s/MiniscriptKey/Key/g on all source files.

Notes

This is an invasive change, I will not be offended if review is a simple 'no'. Outside the miniscript library users can still do miniscript::Key if the trait is too terse for their taste.

@sanket1729

Copy link
Copy Markdown
Member

Over time, I have been liking shorter names. I see the pros and cons of this. I am okay with MsKey, will not oppose to Key if @apoelstra is okay with it.

Outside the miniscript library users can still do miniscript::Key

Deriving MiniscriptKey is only for super advanced users. We don't expect regular users to it.

@apoelstra

Copy link
Copy Markdown
Member

I'm fine with Key, although because it needs to be imported (you have to use use miniscript::Key to get the trait methods, you can't use the fully-qualified name without a lot of noise) I worry that it may conflict with other key types.

On the other hand since edition 2018 you can do use miniscript::Key as _ right?

@tcharding

Copy link
Copy Markdown
Member Author

Light bulb moment, what about we add this as well?

/// Export a type alias to make usage of the library more ergonomic since the
/// `Key` trait name is so generic that it may cause naming conflicts.
pub use Key as MiniscriptKey;

@apoelstra

Copy link
Copy Markdown
Member

oo, I like it! ok, ACK renaming to Key, along with this shim (which also helps with backward compatibility)

@tcharding
tcharding force-pushed the rename-miniscript-key branch from 3dffa08 to 44791dd Compare May 1, 2022 23:48
@tcharding

Copy link
Copy Markdown
Member Author

Changes in force push:

  • Rebase on master
  • Add the 'trait alias' as described above

@tcharding
tcharding force-pushed the rename-miniscript-key branch 2 times, most recently from 7c40009 to c3c5959 Compare May 3, 2022 00:15
@tcharding

tcharding commented May 3, 2022

Copy link
Copy Markdown
Member Author

In the same vein we could shorten ScriptContext -> Context. This would give us, e.g.,

impl<P: Key, C: Context> fmt::Display for Miniscript<P, C> {

Above includes shorter generic identifiers also, I don't see why we use Pk and Ctx when P and C are perfectly clear?

apoelstra
apoelstra previously approved these changes May 3, 2022

@apoelstra apoelstra left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ACK c3c5959

This is gonna cause a lot of merge conflicts..

@tcharding

Copy link
Copy Markdown
Member Author

This is gonna cause a lot of merge conflicts..

Yes. This PR can be re-done at anytime. It requires only a single command (a shell alias I have) to search-and-replace MiniscriptKey with Key. Please feel free to leave this queued up for a time that is suitable in relation to other open PRs. No rush whatsoever.

@sanket1729

Copy link
Copy Markdown
Member

Agreed, keeping this as the last PR to merge.

@tcharding

Copy link
Copy Markdown
Member Author

Making this draft because I do not know how to time it, it can be done mechanically at any stage if/when we feel its needed.

@tcharding
tcharding marked this pull request as draft May 17, 2022 22:35
One of the main traits in this lib is `MiniscriptKey`, we can shorten
this to `Key` with no loss of meaning. This makes the whole codebase
more terse. Terse code, if clear, is easier to read because there is
less clutter. Also terseness assists the formatter and can reduce lines
of code.

This patch is the result of running

`s/MiniscriptKey/Key/g` on all source files and then running the
formatter.

To preserve backwards compatibility and make the library more
ergonomical to use; add a 'trait alias' of `MiniscriptKey` -> `Key`.
@tcharding

Copy link
Copy Markdown
Member Author

Too invasive.

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.

3 participants