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

Move the mastodon/*_cli files to mastodon/cli/* #24139

Merged
merged 1 commit into from May 23, 2023

Conversation

mjankowski
Copy link
Contributor

I opened this draft -- #24134 -- and almost immediately realized the diff is challenging to review because so much changed/moved.

Opening this as an alternative to capture the bear minimum of file movements and changes here -- and will rebase that one for further refactor/specs/etc.

I think the only things here that are not straight renames or path adjustments are:

  • The former lib/cli.rb went to lib/mastodon/cli/main.rb and the tootctl script changed to point to it. This was necessary because once CLI is a module name for all the subcommands, it can't also be a class name for the script entry point.
  • The former CLIHelper is now CLI::Helper

I think everything else is just pure file movement and changing require paths.

@github-actions
Copy link
Contributor

This pull request has merge conflicts that must be resolved before it can be merged.

@github-actions
Copy link
Contributor

This pull request has resolved merge conflicts and is ready for review.

@nschonni
Copy link
Contributor

Since it's moving, it almost seems like it could be renamed it to lib/tootctl inside a separate TootCtl namespace to match how it's externally referenced

@github-actions
Copy link
Contributor

This pull request has merge conflicts that must be resolved before it can be merged.

@github-actions
Copy link
Contributor

This pull request has resolved merge conflicts and is ready for review.

@github-actions
Copy link
Contributor

This pull request has merge conflicts that must be resolved before it can be merged.

@github-actions
Copy link
Contributor

This pull request has resolved merge conflicts and is ready for review.

@mjankowski mjankowski force-pushed the move-cli-classes branch 2 times, most recently from 9c0fd6b to 5c48298 Compare March 27, 2023 16:22
@github-actions
Copy link
Contributor

This pull request has merge conflicts that must be resolved before it can be merged.

@github-actions
Copy link
Contributor

github-actions bot commented Apr 8, 2023

This pull request has resolved merge conflicts and is ready for review.

@github-actions
Copy link
Contributor

This pull request has merge conflicts that must be resolved before it can be merged.

@github-actions
Copy link
Contributor

This pull request has resolved merge conflicts and is ready for review.

@github-actions
Copy link
Contributor

This pull request has merge conflicts that must be resolved before it can be merged.

@github-actions
Copy link
Contributor

github-actions bot commented May 4, 2023

This pull request has resolved merge conflicts and is ready for review.

@mjankowski mjankowski force-pushed the move-cli-classes branch 2 times, most recently from 8433172 to 039afc0 Compare May 9, 2023 16:40
@mjankowski mjankowski force-pushed the move-cli-classes branch 2 times, most recently from 0afdf42 to a7c5f16 Compare May 18, 2023 16:05
@renchap renchap added the refactoring Improving code quality label May 19, 2023
@github-actions
Copy link
Contributor

This pull request has merge conflicts that must be resolved before it can be merged.

@github-actions
Copy link
Contributor

This pull request has resolved merge conflicts and is ready for review.

@mjankowski
Copy link
Contributor Author

@Gargron @ClearlyClaire I'd love to get a review on this one.

If approved, I want to rebase the other refactor+spec PR I have open against this post-file-move and start to get some of the CLI coverage improvements in. By raw LOC, the CLI classes are the largest contributor to missing coverage now.

If the move is not approved, I can back out some of the spec+refactor stuff from the other PR and open more PRs against the current file names.

Finally, I tried to make this as minimal a move/rename as possible, but if you see an even simpler way to do it let me know and I'll modify as desired.

@github-actions
Copy link
Contributor

This pull request has merge conflicts that must be resolved before it can be merged.

@github-actions
Copy link
Contributor

This pull request has resolved merge conflicts and is ready for review.

Copy link
Contributor

@ClearlyClaire ClearlyClaire left a comment

Choose a reason for hiding this comment

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

The new module/class/file names are definitely cleaner, but I'm not sure how much renaming this is worth the noise.

I'm tentatively for this, but I'd like @Gargron's opinion on it.

@mjankowski
Copy link
Contributor Author

The new module/class/file names are definitely cleaner, but I'm not sure how much renaming this is worth the noise.

I'm expecting that this one-time move/rename is the noisiest lowest value (but necessary) part and the benefits show up in future PRs adding common base class, refactoring the helper, etc.

I'm tentatively for this, but I'd like @Gargron's opinion on it.

Agreed, but FWIW, work here done in part after this comment: #23978 (comment)

@Gargron Gargron merged commit b6b4ea4 into mastodon:main May 23, 2023
28 checks passed
@mjankowski mjankowski deleted the move-cli-classes branch May 23, 2023 14:23
skerit pushed a commit to 11ways/mastodon that referenced this pull request Jul 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Improving code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants