Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd: Remove snake case deprecation warning #626

Merged
merged 3 commits into from
Apr 7, 2023

Conversation

thanethomson
Copy link
Contributor

While working on #625 I discovered this. We have aliases for both types of commands, so I don't think there's any need to issue a warning here.


PR checklist

  • Tests written/updated
  • Changelog entry added in .changelog (we use unclog to manage our changelog)
  • Updated relevant documentation (docs/ or spec/) and code comments

@thanethomson thanethomson added the P:tech-debt Priority: Technical debt that needs to be paid off to enable us to move faster, reliably label Mar 31, 2023
@thanethomson thanethomson requested a review from a team as a code owner March 31, 2023 13:33
@thanethomson thanethomson mentioned this pull request Mar 31, 2023
3 tasks
@thanethomson thanethomson added backport-to-v0.34.x Tell Mergify to backport the PR to v0.34.x backport-to-v0.37.x Tell Mergify to backport the PR to v0.37.x backport-to-v0.38.x Tell Mergify to backport the PR to v0.38.x labels Mar 31, 2023
cmd/cometbft/commands/reset.go Outdated Show resolved Hide resolved
@sergio-mena
Copy link
Contributor

The changes make sense technically. However, I have the impression that they added the "deprecate snake case warning" because they were planning to remove the (snake_case) aliases in a subsequent breaking release.
(take a look at this and this)

@thanethomson
Copy link
Contributor Author

Do we still want to remove the snake_case aliases then? Or shouldn't we just offer both? (kebab-case is easier to type when using the CLI)

And wasn't the snake_case deprecation part of the broader effort to kebab-case the config file?

@sergio-mena
Copy link
Contributor

Do we still want to remove the snake_case aliases then? Or shouldn't we just offer both? (kebab-case is easier to type when using the CLI)

I think that deserves a (maybe short) synchronous discussion? I've added a point for tomorrow's internal meeting. I apologize for adding a point to a meeting I'll probably not be able to attend :-|
But I am OK with whatever you folks decide.

And wasn't the snake_case deprecation part of the broader effort to kebab-case the config file?

I think so, although, by the age of tendermint/tendermint#5786 and tendermint/tendermint#5777, I'd say they first tackled this, and later (as of v0.35.x) they tackled the config.toml.

Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
@thanethomson thanethomson force-pushed the thane/625-rm-snakecase-deprecation branch from e5f91a0 to 4e4b7b9 Compare April 6, 2023 17:30
@thanethomson
Copy link
Contributor Author

thanethomson commented Apr 6, 2023

I think that deserves a (maybe short) synchronous discussion?

We spoke about it in the team call today and we decided to:

  1. Continue to offer kebab-case CLI commands as the default, as it's generally more ergonomic to type than snake_case when using the CLI.
  2. Continue to provide snake_case CLI aliases to not break folks who depend on them.
  3. Remove the deprecation warning.

@thanethomson thanethomson merged commit c1214b2 into main Apr 7, 2023
20 checks passed
@thanethomson thanethomson deleted the thane/625-rm-snakecase-deprecation branch April 7, 2023 10:57
mergify bot pushed a commit that referenced this pull request Apr 7, 2023
* cmd: Remove snake case deprecation warning

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* cmd: Add snake_case aliases for commands that do not have

Signed-off-by: Thane Thomson <connect@thanethomson.com>

---------

Signed-off-by: Thane Thomson <connect@thanethomson.com>
(cherry picked from commit c1214b2)
mergify bot pushed a commit that referenced this pull request Apr 7, 2023
* cmd: Remove snake case deprecation warning

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* cmd: Add snake_case aliases for commands that do not have

Signed-off-by: Thane Thomson <connect@thanethomson.com>

---------

Signed-off-by: Thane Thomson <connect@thanethomson.com>
(cherry picked from commit c1214b2)

# Conflicts:
#	cmd/cometbft/commands/root.go
mergify bot pushed a commit that referenced this pull request Apr 7, 2023
* cmd: Remove snake case deprecation warning

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* cmd: Add snake_case aliases for commands that do not have

Signed-off-by: Thane Thomson <connect@thanethomson.com>

---------

Signed-off-by: Thane Thomson <connect@thanethomson.com>
(cherry picked from commit c1214b2)

# Conflicts:
#	cmd/cometbft/commands/reindex_event.go
#	cmd/cometbft/commands/root.go
thanethomson added a commit that referenced this pull request Apr 7, 2023
* cmd: Remove snake case deprecation warning

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* cmd: Add snake_case aliases for commands that do not have

Signed-off-by: Thane Thomson <connect@thanethomson.com>

---------

Signed-off-by: Thane Thomson <connect@thanethomson.com>
(cherry picked from commit c1214b2)

Co-authored-by: Thane Thomson <connect@thanethomson.com>
thanethomson added a commit that referenced this pull request Apr 7, 2023
* cmd: Remove snake case deprecation warning (#626)

* cmd: Remove snake case deprecation warning

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* cmd: Add snake_case aliases for commands that do not have

Signed-off-by: Thane Thomson <connect@thanethomson.com>

---------

Signed-off-by: Thane Thomson <connect@thanethomson.com>
(cherry picked from commit c1214b2)

# Conflicts:
#	cmd/cometbft/commands/root.go

* Resolve conflicts

Signed-off-by: Thane Thomson <connect@thanethomson.com>

---------

Signed-off-by: Thane Thomson <connect@thanethomson.com>
Co-authored-by: Thane Thomson <connect@thanethomson.com>
thanethomson added a commit that referenced this pull request Apr 7, 2023
* cmd: Remove snake case deprecation warning (#626)

* cmd: Remove snake case deprecation warning

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* cmd: Add snake_case aliases for commands that do not have

Signed-off-by: Thane Thomson <connect@thanethomson.com>

---------

Signed-off-by: Thane Thomson <connect@thanethomson.com>
(cherry picked from commit c1214b2)

# Conflicts:
#	cmd/cometbft/commands/reindex_event.go
#	cmd/cometbft/commands/root.go

* Resolve conflicts

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* cmd: Restore short description for reindex-event to conform to v0.34 convention

Signed-off-by: Thane Thomson <connect@thanethomson.com>

---------

Signed-off-by: Thane Thomson <connect@thanethomson.com>
Co-authored-by: Thane Thomson <connect@thanethomson.com>
thanethomson added a commit that referenced this pull request May 6, 2023
* cmd: Remove snake case deprecation warning

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* cmd: Add snake_case aliases for commands that do not have

Signed-off-by: Thane Thomson <connect@thanethomson.com>

---------

Signed-off-by: Thane Thomson <connect@thanethomson.com>
roy-dydx pushed a commit to dydxprotocol/cometbft that referenced this pull request Jul 11, 2023
…ometbft#671)

* cmd: Remove snake case deprecation warning (cometbft#626)

* cmd: Remove snake case deprecation warning

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* cmd: Add snake_case aliases for commands that do not have

Signed-off-by: Thane Thomson <connect@thanethomson.com>

---------

Signed-off-by: Thane Thomson <connect@thanethomson.com>
(cherry picked from commit c1214b2)

# Conflicts:
#	cmd/cometbft/commands/root.go

* Resolve conflicts

Signed-off-by: Thane Thomson <connect@thanethomson.com>

---------

Signed-off-by: Thane Thomson <connect@thanethomson.com>
Co-authored-by: Thane Thomson <connect@thanethomson.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-to-v0.34.x Tell Mergify to backport the PR to v0.34.x backport-to-v0.37.x Tell Mergify to backport the PR to v0.37.x backport-to-v0.38.x Tell Mergify to backport the PR to v0.38.x P:tech-debt Priority: Technical debt that needs to be paid off to enable us to move faster, reliably
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants