Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Rename --pruning and --keep-blocks to be more similar to one another #11934

Merged

Conversation

wirednkod
Copy link
Contributor

@wirednkod wirednkod commented Jul 28, 2022

This PR is renaming flags --keep-blocks and --prunning to --blocks-pruning and --state-prunning` accordingly. This makes the flags similar to each other and avoids confusion.

Fixes #11907

polkadot companion: paritytech/polkadot#5863
cumulus companion: paritytech/cumulus#1514

@wirednkod wirednkod added A0-please_review Pull request needs code review. B5-clientnoteworthy C1-low PR touches the given topic and has a low impact on builders. D2-notlive 💤 PR contains changes in a runtime directory that is not deployed to a chain that requires an audit. labels Jul 28, 2022
@wirednkod wirednkod requested review from kianenigma and bkchr July 28, 2022 17:20
@shawntabrizi
Copy link
Member

Is this backwards compatible?

@bkchr
Copy link
Member

bkchr commented Jul 28, 2022

Is this backwards compatible?

No

@wirednkod
Copy link
Contributor Author

Is this backwards compatible?

Should it be?

@bkchr
Copy link
Member

bkchr commented Jul 29, 2022

Yes, sounds like a good idea. We should rename the in code variable and add some alias for the old names.

@kianenigma
Copy link
Contributor

Yes, sounds like a good idea. We should rename the in code variable and add some alias for the old names.

and emit a warning if the old versions are being used about potential deprecation, then in a few months time we can remove the old name entirely.

@bkchr
Copy link
Member

bkchr commented Aug 2, 2022

I don't think that we can find out if they are being passed. Okay, we could probably do it but it would be very complicated. Clap wouldn't show the alises in the --help. Meaning, the usage probably disappears automatically over time and keeping the alias will also not be that complicated.

@wirednkod
Copy link
Contributor Author

wirednkod commented Aug 5, 2022

Yes, sounds like a good idea. We should rename the in code variable and add some alias for the old names.

I added alias in clap instead of renaming the in code variable and it works just fine.
I agree, though, that renaming the variable, even if its a little bit more work, is valid in order not to leave traits of the past.

Now using both aliases the error message informs only for the new one:

➜  substrate ✗ ./target/release/substrate --dev --keep-blocks asfda
error: Invalid value "asfda" for '--blocks-pruning <COUNT>': invalid digit found in string

For more information try --help
➜  substrate ✗ ./target/release/substrate --dev --blocks-pruning asfda
error: Invalid value "asfda" for '--blocks-pruning <COUNT>': invalid digit found in string

@wirednkod
Copy link
Contributor Author

@bkchr there is a test failing due to this. What can I do for this? Open a new PR in polkadot or... ?

@bkchr
Copy link
Member

bkchr commented Aug 5, 2022

@bkchr there is a test failing due to this. What can I do for this? Open a new PR in polkadot or... ?

You need to create a companion.

@wirednkod
Copy link
Contributor Author

@bkchr there is a test failing due to this. What can I do for this? Open a new PR in polkadot or... ?

You need to create a companion.

Done here

Copy link
Member

@shawntabrizi shawntabrizi left a comment

Choose a reason for hiding this comment

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

looks reasonable to me!

Thanks for keeping it backwards compatible

@bkchr
Copy link
Member

bkchr commented Aug 6, 2022

@wirednkod missing cumulus companion.

@wirednkod
Copy link
Contributor Author

wirednkod commented Aug 8, 2022

cumulus companion: paritytech/cumulus#1514

Thank you @bkchr

Done here

@bkchr
Copy link
Member

bkchr commented Aug 8, 2022

bot merge

@paritytech-processbot paritytech-processbot bot merged commit 8574647 into master Aug 8, 2022
@paritytech-processbot paritytech-processbot bot deleted the nik-rename-prunning-and-keep-blocks-flags branch August 8, 2022 09:31
DaviRain-Su pushed a commit to octopus-network/substrate that referenced this pull request Aug 23, 2022
…aritytech#11934)

* rename prunning and keep-blocks flags

* Add aliases in keep-blocks and pruning for backward compatibility

* Rename in code variables from  and  to  and
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders. D2-notlive 💤 PR contains changes in a runtime directory that is not deployed to a chain that requires an audit.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rename --pruning and --keep-blocks to be more similar to one another.
4 participants