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

unitctl: rename app to apps #1424

Merged
merged 2 commits into from
Sep 16, 2024
Merged

Conversation

javorszky
Copy link
Contributor

This PR achieves three separate things:

  • cleans up some grouping of args with its commands to separate it from the next commands
  • renamed the app to apps. Clap picks up the name of the enum as the subcommand name absent a command argument. I chose to rename the enum rather than add an option
  • adds the apps line to the readme

It does not fix the whole of the readme, that's a separate issue and PR.

Closes #1423

Copy link
Collaborator

@callahad callahad 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 probably worth cleaning up the references to singular unitctl app in the README.md as part of this PR

@javorszky
Copy link
Contributor Author

I think it's probably worth cleaning up the references to singular unitctl app in the README.md as part of this PR

Fixed in f5a4e44

Copy link
Collaborator

@callahad callahad left a comment

Choose a reason for hiding this comment

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

Please squash together the 3 commits that aren't whitespace/reordering lines, otherwise the net effect LGTM.

@javorszky
Copy link
Contributor Author

Please squash together the 3 commits that aren't whitespace/reordering lines, otherwise the net effect LGTM.

squashed

Copy link
Member

@ac000 ac000 left a comment

Choose a reason for hiding this comment

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

 unitctl: rename app -> apps, fix readme

Signed-off-by: Gabor Javorszky <[email protected]>

Prefix should really be tools/unitctl for consistency, but I can live with just unitctl...

Copy link
Member

@ac000 ac000 left a comment

Choose a reason for hiding this comment

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

 whitespace: group and order command args

Signed-off-by: Gabor Javorszky <[email protected]>

Prefix should be tools/unitctl (or just unitctl depending on what you decide for the other commit). whitespace is little generic and doesn't really tell me anything...

Maybe just something like

unitctl: Whitespace fixes

@javorszky
Copy link
Contributor Author

@ac000 I fixed the commit subjects 🙂

@ac000
Copy link
Member

ac000 commented Sep 16, 2024

Rebase with master.

@ac000 ac000 merged commit 5e8a689 into nginx:master Sep 16, 2024
7 checks passed
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.

unitctl: rename app to apps
4 participants