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

Add a --edit flag #5419

Merged
merged 2 commits into from
Jan 23, 2025
Merged

Add a --edit flag #5419

merged 2 commits into from
Jan 23, 2025

Conversation

MrJohz
Copy link
Contributor

@MrJohz MrJohz commented Jan 21, 2025

The --edit flag forces the editor to be shown, even if it would have been hidden due to the --no-edit flag or another flag that implies it (such as --message or --stdin).

This fixes #5379

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

Copy link

google-cla bot commented Jan 21, 2025

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@MrJohz MrJohz force-pushed the add-edit-flag branch 2 times, most recently from c7061b2 to e7b2b20 Compare January 21, 2025 15:26
@arxanas
Copy link
Contributor

arxanas commented Jan 21, 2025

In other contexts, --edit means "switch to the commit as the new working copy and start editing it", which is inconsistent with its usage here (and the current usage of --no-edit). Maybe it should be something like --editor/--no-editor?

@MrJohz
Copy link
Contributor Author

MrJohz commented Jan 21, 2025

That makes sense to me and I'm happy to make the change. To briefly play devil's advocate:

  • That makes this a breaking change for anyone who's already using --no-edit in scripts etc
  • Both Git and Mercurial use --edit (and --no-edit for Git) to invoke/avoid invoking the editor.

But internal consistency is more important than consistency with those other tools, so I'm happy to make the change if you/other people would prefer it that way.

@PhilipMetzger
Copy link
Contributor

PhilipMetzger commented Jan 21, 2025

What about --open-editor as a flag? Also adhere to the commit style outlined here: https://github.com/jj-vcs/jj/blob/main/docs/contributing.md#code-reviews

Copy link
Contributor

@arxanas arxanas left a comment

Choose a reason for hiding this comment

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

Looks good to me overall, thanks for implementing this! Also be sure to adjust the commit message as per @PhilipMetzger's comment.

Re --editor vs --no-editor, I also asked in this Discord message. It seems like there's many factors to consider, so let's not block this PR on that. I'll open a separate discussion.

cli/src/commands/describe.rs Show resolved Hide resolved
cli/src/commands/describe.rs Outdated Show resolved Hide resolved
cli/src/commands/describe.rs Outdated Show resolved Hide resolved
cli/tests/test_describe_command.rs Outdated Show resolved Hide resolved
cli/tests/test_describe_command.rs Outdated Show resolved Hide resolved
@MrJohz MrJohz force-pushed the add-edit-flag branch 6 times, most recently from ddef46c to 2c947a5 Compare January 22, 2025 11:44
@MrJohz
Copy link
Contributor Author

MrJohz commented Jan 22, 2025

What's the convention here for who merges, when that happens, etc? I think this is ready to go now. @arxanas

@PhilipMetzger
Copy link
Contributor

PhilipMetzger commented Jan 22, 2025

The procedure currently is that Martin invites you to the jj-vcs/contributors team and then you can land it yourself.

cli/src/commands/describe.rs Outdated Show resolved Hide resolved
cli/tests/test_describe_command.rs Outdated Show resolved Hide resolved
cli/tests/test_describe_command.rs Show resolved Hide resolved
cli/tests/test_describe_command.rs Outdated Show resolved Hide resolved
cli/tests/test_describe_command.rs Outdated Show resolved Hide resolved
The --edit flag forces the editor to be shown, even if it would have
been hidden due to the --no-edit flag or another flag that implies it
(such as --message or --stdin).
@MrJohz MrJohz enabled auto-merge January 23, 2025 16:03
@MrJohz MrJohz added this pull request to the merge queue Jan 23, 2025
Merged via the queue into jj-vcs:main with commit 55d1c17 Jan 23, 2025
19 checks passed
@MrJohz MrJohz deleted the add-edit-flag branch January 23, 2025 16:25
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.

FR: --edit flag for describe
4 participants