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.

Sign up for GitHub

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

Jump to bottom

Switch ui.movement.edit to a simple boolean. #4302

Closed
wants to merge 2 commits into from
Closed

Switch ui.movement.edit to a simple boolean. #4302

wants to merge 2 commits into from

Conversation

essiene
Copy link
Contributor

@essiene essiene commented Aug 19, 2024 •
edited
Loading

  • Consensus is that auto mode where we try to infer when to
    switch to edit mode, should be removed. See thread in #4283
  • Change ui.movement.edit to a simple boolean flag.
    true - means edit mode
    false- new+squash mode
  • Remove MovementSettings and related config elements as things
    are simpler now.
  • Simplify code that used the previous enough.
  • Update tests that assumed edit mode inference, to specify --edit explicitly.
  • Update tests to update how we specify ui.movement.edit

Part of #3947

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

@essiene essiene force-pushed the essiene/push-utlmxzpwopxr branch from 1788095 to 4867396 Compare August 19, 2024 11:11
@essiene essiene force-pushed the essiene/push-pmsvoxtkupkp branch from 22f2b28 to 0cb4fdc Compare August 19, 2024 11:11
@essiene essiene mentioned this pull request Aug 19, 2024
Merged
@essiene essiene force-pushed the essiene/push-pmsvoxtkupkp branch from 0cb4fdc to 80cf4d2 Compare August 19, 2024 11:44
@essiene essiene marked this pull request as ready for review August 19, 2024 11:46
@essiene essiene force-pushed the essiene/push-utlmxzpwopxr branch from 4867396 to 2a5ea86 Compare August 19, 2024 12:09
@essiene essiene force-pushed the essiene/push-pmsvoxtkupkp branch from 80cf4d2 to 666be54 Compare August 19, 2024 12:09
@essiene essiene force-pushed the essiene/push-utlmxzpwopxr branch from 2a5ea86 to bda122e Compare August 19, 2024 14:49
@essiene essiene force-pushed the essiene/push-pmsvoxtkupkp branch from 666be54 to 41714af Compare August 19, 2024 14:49
yuja
yuja approved these changes Aug 20, 2024
View reviewed changes
Copy link
Contributor

@yuja yuja 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 it's good to squash this into the previous PR, but I don't have strong opinion.

Thanks.

CHANGELOG.md Show resolved Hide resolved
cli/src/movement_util.rs Show resolved Hide resolved
cli/tests/test_next_prev_commands.rs Outdated Show resolved Hide resolved
cli/tests/test_next_prev_commands.rs Outdated Show resolved Hide resolved
@essiene essiene force-pushed the essiene/push-utlmxzpwopxr branch from bda122e to 8eb69a2 Compare August 20, 2024 12:58
@essiene essiene force-pushed the essiene/push-pmsvoxtkupkp branch from 41714af to 7173dd9 Compare August 20, 2024 12:58
essiene added a commit that referenced this pull request Aug 20, 2024
@essiene
next/prev: Add config flag to control prev/next edit behaviour.
d421732 
The flag is a tristate flag where:
  - Auto - Maintain current behaviour. This edits if
    the wc parent is not a head commit. Else, it will
    create a new commit on the parent of the wc in
    the direction of movement.
  - Always - Always edit
  - Never - Never edit, prefer the new+squash workflow.

Also add a `--no-edit` flag as the explicit inverse of `--edit` and
ensure both flags take precedence over the config.

NOTE: There is a follow up #4302 that changes the config knob to a boolean.
I'm keeping that as a separate PR for review clarity as that changes some
tests, so keeping them separate makes it easier to see what's changing.

Part of #3947
@essiene essiene force-pushed the essiene/push-utlmxzpwopxr branch from 8eb69a2 to d421732 Compare August 20, 2024 13:48
@essiene
next/prev: Add config flag to control prev/next edit behaviour.
9237e35 
The flag is a tristate flag where:
  - Auto - Maintain current behaviour. This edits if
    the wc parent is not a head commit. Else, it will
    create a new commit on the parent of the wc in
    the direction of movement.
  - Always - Always edit
  - Never - Never edit, prefer the new+squash workflow.

Also add a `--no-edit` flag as the explicit inverse of `--edit` and
ensure both flags take precedence over the config.

NOTE: There is a follow up #4302 that changes the config knob to a boolean.
I'm keeping that as a separate PR for review clarity as that changes some
tests, so keeping them separate makes it easier to see what's changing.

Part of #3947
@essiene essiene force-pushed the essiene/push-utlmxzpwopxr branch from d421732 to 9237e35 Compare August 20, 2024 14:00
@essiene
Switch ui.movement.edit to a simple boolean.
f0d6c53 
* Consensus on #4283 is that `auto` mode where we try to infer when to
  switch to `edit mode`, should be removed.
* Change `ui.movement.edit` to a simple boolean flag.
  true - means edit mode
  false- new+squash mode
* Remove `MovementSettings` and related config elements as things
  are simpler now.
* Simplify code that used the previous enough.
* Update tests that assumed edit mode inference, to specify `--edit` explicitly.
* Update tests to update how we specify `ui.movement.edit`

Part of #3947
@essiene essiene force-pushed the essiene/push-pmsvoxtkupkp branch from 7173dd9 to f0d6c53 Compare August 20, 2024 14:07
@essiene
Copy link
Contributor Author

essiene commented Aug 20, 2024

I think it's good to squash this into the previous PR, but I don't have strong opinion.

Thanks.

Yeah, I'll go ahead and squash them now that both are approved.

Thanks.

@essiene essiene force-pushed the essiene/push-utlmxzpwopxr branch from 9237e35 to 2afba89 Compare August 20, 2024 14:14
@essiene
Copy link
Contributor Author

essiene commented Aug 20, 2024

This has been squashed into #4283

@essiene essiene closed this Aug 20, 2024
essiene added a commit that referenced this pull request Aug 20, 2024
@essiene
next/prev: Add config flag to control prev/next edit behaviour.
3742158 
* We started with a tristate flag where:
    - Auto - Maintain current behaviour. This edits if
      the wc parent is not a head commit. Else, it will
      create a new commit on the parent of the wc in
      the direction of movement.
    - Always - Always edit
    - Never - Never edit, prefer the new+squash workflow.
  However, consensus the review thread is that `auto` mode where we try to infer when to
  switch to `edit mode`, should be removed. So `ui.movement.edit` is a boolean flag now.
    - true: edit mode
    - false: new+squash mode
* Also add a `--no-edit` flag as the explicit inverse of `--edit` and
  ensure both flags take precedence over the config.
* Update tests that assumed edit mode inference, to specify `--edit` explicitly.

NOTE: #4302 was squashed into this commit, so see that closed PR for review history.

Part of #3947
@essiene essiene deleted the essiene/push-pmsvoxtkupkp branch August 20, 2024 14:29
essiene added a commit that referenced this pull request Aug 20, 2024
@essiene
next/prev: Add config flag to control prev/next edit behaviour.
bb018a5 
* We started with a tristate flag where:
    - Auto - Maintain current behaviour. This edits if
      the wc parent is not a head commit. Else, it will
      create a new commit on the parent of the wc in
      the direction of movement.
    - Always - Always edit
    - Never - Never edit, prefer the new+squash workflow.
  However, consensus the review thread is that `auto` mode where we try to infer when to
  switch to `edit mode`, should be removed. So `ui.movement.edit` is a boolean flag now.
    - true: edit mode
    - false: new+squash mode
* Also add a `--no-edit` flag as the explicit inverse of `--edit` and
  ensure both flags take precedence over the config.
* Update tests that assumed edit mode inference, to specify `--edit` explicitly.

NOTE: #4302 was squashed into this commit, so see that closed PR for review history.

Part of #3947
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yuja yuja yuja approved these changes

@martinvonz martinvonz Awaiting requested review from martinvonz

@PhilipMetzger PhilipMetzger Awaiting requested review from PhilipMetzger

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@essiene @yuja