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

next/prev: refactor movement utilities into cli/src/movement_utils.rs #4282

Merged
merged 1 commit into from
Aug 16, 2024
Merged

next/prev: refactor movement utilities into cli/src/movement_utils.rs #4282

merged 1 commit into from
Aug 16, 2024

Conversation

essiene
Copy link
Contributor

@essiene essiene commented Aug 15, 2024 •
edited
Loading

The code in both cli/src/commands/{next,prev}.rs is identical except for the direction of movement. This PR pull the parts that make sense out into cli/src/movement_util.rs so it's easier to see the differences. No new behavious is introduced.

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 marked this pull request as ready for review August 15, 2024 21:37
martinvonz
martinvonz reviewed Aug 16, 2024
View reviewed changes
cli/src/movement_util.rs Outdated Show resolved Hide resolved
cli/src/movement_util.rs Outdated Show resolved Hide resolved
cli/src/movement_util.rs Outdated Show resolved Hide resolved
cli/src/commands/next.rs Outdated Show resolved Hide resolved
martinvonz
martinvonz reviewed Aug 16, 2024
View reviewed changes
Copy link
Member

@martinvonz martinvonz left a comment

Choose a reason for hiding this comment

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

nit: I think "no-op" and "Refactor" is redundant. Refactoring always has (or should have) no visible effect on the behavior. So e.g. "next/prev: refactor ..." would be slightly clearer to me, and "This no-op PR" sounds better as "This refactoring commit/patch" sounds better to me (it's clearer about the sense in which it's a no-op).

@essiene essiene changed the title no-op: Refactor movement utilities into cli/src/movement_utils.rs Refactor movement utilities into cli/src/movement_utils.rs Aug 16, 2024
@essiene essiene changed the title Refactor movement utilities into cli/src/movement_utils.rs next/prev: refactor movement utilities into cli/src/movement_utils.rs Aug 16, 2024
@essiene essiene force-pushed the essiene/push-uxmkksrpywlq branch from da56062 to 4b6b93c Compare August 16, 2024 15:30
@PhilipMetzger
Copy link
Contributor

PhilipMetzger commented Aug 16, 2024 •
edited
Loading

I had a similar approach here: PhilipMetzger@9c37e18, although I renamed the directions to just Direction::Ancestors and Direction::Descendants.

@essiene
Copy link
Contributor Author

essiene commented Aug 16, 2024

nit: I think "no-op" and "Refactor" is redundant. Refactoring always has (or should have) no visible effect on the behavior. So e.g. "next/prev: refactor ..." would be slightly clearer to me, and "This no-op PR" sounds better as "This refactoring commit/patch" sounds better to me (it's clearer about the sense in which it's a no-op).

Makes sense. Reworded.

@essiene essiene force-pushed the essiene/push-lkzyxpuyumyq branch 2 times, most recently from 8ab778f to 67a730c Compare August 16, 2024 19:29
@essiene essiene force-pushed the essiene/push-uxmkksrpywlq branch from 4b6b93c to 50395e7 Compare August 16, 2024 19:29
martinvonz
martinvonz approved these changes Aug 16, 2024
View reviewed changes
cli/src/movement_util.rs Outdated Show resolved Hide resolved
Base automatically changed from essiene/push-uxmkksrpywlq to main August 16, 2024 20:28
@essiene essiene force-pushed the essiene/push-lkzyxpuyumyq branch 3 times, most recently from e7ccd6a to e34ebe1 Compare August 16, 2024 21:53
@essiene
next/prev: refactor movement utilities into cli/src/movement_utils.rs
32c715f 
The code in both cli/src/commands/{next,prev}.rs is identical except
for the direction of movement. This commit pull the parts that make
sense out into cli/src/movement_util.rs so it's easier to see the
differences.

Part of #3947
@essiene essiene force-pushed the essiene/push-lkzyxpuyumyq branch from e34ebe1 to 32c715f Compare August 16, 2024 22:07
@essiene essiene enabled auto-merge (rebase) August 16, 2024 22:17
@essiene essiene merged commit 237b41e into main Aug 16, 2024
31 checks passed
@essiene essiene deleted the essiene/push-lkzyxpuyumyq branch August 16, 2024 22:21
yuja
yuja reviewed Aug 17, 2024
View reviewed changes
cli/src/movement_util.rs Show resolved Hide resolved
cli/src/movement_util.rs Show resolved Hide resolved
cli/src/movement_util.rs Show resolved Hide resolved
@essiene essiene mentioned this pull request Aug 18, 2024
Merged
4 tasks
essiene added a commit that referenced this pull request Aug 18, 2024
@essiene
Follow ups on post-submission comments for #4282.
66ac00c 
* Derive a bunch of standard and useful traits for `movement_util::Direction`
  as it is a simple type. Importantly `Copy`.
* Return `&'static str` from Direction.cmd()
* Refactor out `MovementArgs` to reduce the number of arguments
  to `movement_util::move_to_commit`.
* Implement `From<&NextArgs/&PrevArgs>` for MovementArgs

Part of #3947
essiene added a commit that referenced this pull request Aug 18, 2024
@essiene
Follow ups on post-submission comments for #4282.
39d1215 
* Derive a bunch of standard and useful traits for `movement_util::Direction`
  as it is a simple type. Importantly `Copy`.
* Return `&'static str` from Direction.cmd()
* Refactor out `MovementArgs` to reduce the number of arguments
  to `movement_util::move_to_commit`.
* Implement `From<&NextArgs/&PrevArgs>` for MovementArgs

Part of #3947
essiene added a commit that referenced this pull request Aug 18, 2024
@essiene
Follow ups on post-submission comments for #4282.
db488c1 
* Derive a bunch of standard and useful traits for `movement_util::Direction`
  as it is a simple type. Importantly `Copy`.
* Return `&'static str` from Direction.cmd()
* Refactor out `MovementArgs` to reduce the number of arguments
  to `movement_util::move_to_commit`.
* Implement `From<&NextArgs/&PrevArgs>` for MovementArgs

Part of #3947
essiene added a commit that referenced this pull request Aug 18, 2024
@essiene
Follow ups on post-submission comments for #4282.
ea91cb2 
* Derive a bunch of standard and useful traits for `movement_util::Direction`
  as it is a simple type. Importantly `Copy`.
* Return `&'static str` from Direction.cmd()
* Refactor out `MovementArgs` to reduce the number of arguments
  to `movement_util::move_to_commit`.
* Implement `From<&NextArgs/&PrevArgs>` for MovementArgs

Part of #3947
essiene added a commit that referenced this pull request Aug 18, 2024
@essiene
Follow ups on post-submission comments for #4282.
e2a5c83 
* Derive a bunch of standard and useful traits for `movement_util::Direction`
  as it is a simple type. Importantly `Copy`.
* Return `&'static str` from Direction.cmd()
* Refactor out `MovementArgs` to reduce the number of arguments
  to `movement_util::move_to_commit`.
* Implement `From<&NextArgs/&PrevArgs>` for MovementArgs

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 left review comments

@martinvonz martinvonz martinvonz approved these changes

@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.
4 participants
@essiene @PhilipMetzger @yuja @martinvonz