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

cli: add error stub for jj util mangen #5528

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

arxanas
Copy link
Contributor

@arxanas arxanas commented Jan 31, 2025

Closes #5525.

Checklist

If applicable:

  • I have updated CHANGELOG.md
  • I have updated the documentation (README.md, docs/, demos/)
  • I have updated the config schema (cli/src/config-schema.json)
  • I have added tests to cover my changes

Packagers who are invoking `jj util mangen` will get broken with no advice on how to remediate. Soft-deprecating the command with a warning probably won't help here because package installation is automated, and presumably won't surface those warnings. See the issue linked in the error message string for more detail.
@arxanas arxanas marked this pull request as ready for review January 31, 2025 02:30
Copy link
Contributor

@PhilipMetzger PhilipMetzger left a comment

Choose a reason for hiding this comment

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

I think a full restore should be in state, since the actual PR didn't really follow the "normal" deprecation and removal procedure.

Comment on lines +50 to +51
/// will be removed in a future version.
Mangen,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: missing TODO for the removal in jj 0.30+

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, will add (also I didn't know the next jj version 🤣).

@arxanas
Copy link
Contributor Author

arxanas commented Jan 31, 2025

I think a full restore should be in state.

@PhilipMetzger What does "in state" mean? Does this mean the same as "in order", as in "warranted in this situation"?

the actual PR didn't really follow the "normal" deprecation and removal procedure.

See the commit message for why I think it's not useful to follow the normal deprecation feature. I was going to do that first, but I don't think it improves anything for package maintainers.

@PhilipMetzger
Copy link
Contributor

I think a full restore should be in state.

@PhilipMetzger What does "in state" mean? Does this mean the same as "in order", as in "warranted in this situation"?

Yes, that should've been a "a full restore in order".

See the commit message for why I think it's not useful to follow the normal deprecation feature. I was going to do that first, but I don't think it improves anything for package maintainers.

While it happened with someone who was a packager, we have no data on actual command utilization in the wild. So carving out such a exception for packagers seems really unwise. It also casually breaks our social contract.

@emilazy
Copy link
Contributor

emilazy commented Jan 31, 2025

This is a packaging command so I think applying norms for packagers makes sense? (Even if someone is “packaging by hand”, they’ll do it once per install, so I don’t think this could break someone’s day‐to‐day workflow.)

FWIW as a downstream packager of Jujutsu I would find a hard error for this much more useful than a restored deprecated command.

(Is the long deprecation period for every backwards‐incompatible change written down anywhere?)

@@ -45,6 +46,9 @@ pub(crate) enum UtilCommand {
ConfigSchema(UtilConfigSchemaArgs),
Exec(UtilExecArgs),
Gc(UtilGcArgs),
/// No longer supported; use `jj util install-man-pages` instead. This stub
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like this command should be hidden in the Clap sense. This should also remove it from the docs. TODO: Find the right syntax.

Copy link
Contributor

@ilyagr ilyagr Feb 1, 2025

Choose a reason for hiding this comment

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

I think adding #[command(hide = true)] here will do it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, good point, thanks for raising.

Copy link
Contributor

@ilyagr ilyagr left a comment

Choose a reason for hiding this comment

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

Thank you!

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.

jj util mangen will be broken for packagers in next release
4 participants