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

feat: make rookify execute on run only #88

Merged
merged 3 commits into from
Oct 1, 2024
Merged

feat: make rookify execute on run only #88

merged 3 commits into from
Oct 1, 2024

Conversation

boekhorstb1
Copy link
Contributor

@boekhorstb1 boekhorstb1 commented Sep 25, 2024

  • Add run to execute and run --help per default
  • Add short version for CLI

closes #87

.venv/bin/rookify
usage: Rookify [-h] [-d] [-s] [run]

positional arguments:
  run                Run the migration.

options:
  -h, --help         show this help message and exit
  -d, --dry-run
  -s, --show-states  Show states of the modules.

@boekhorstb1 boekhorstb1 self-assigned this Sep 25, 2024
@NotTheEvilOne
Copy link
Contributor

Dear @boekhorstb1,

I like the intention of not execute Rookify by mistake. Therefore I would love to see --dry-run to be replaced by a --run parameter as like suggested you would construct a command line like rookify --dry-run run to execute Rookify in dry-run mode. This is a bit counter-intuitive.

How about the following changes:

  • Run Rookify in dry-run mode by default (remove dry-run parameter)
  • Add a parameter like --migrate with default False to do the actual work

@boekhorstb1
Copy link
Contributor Author

boekhorstb1 commented Sep 26, 2024

rookify --dry-run run

Hi @NotTheEvilOne !

I think it would look rather like this, if it preflight mode should be used only:

rookify run --dry-run 
# or
rookify run -d

We could add the migrate parameter. But Im not sure how.
Another idea. How about we change it like this:

rookify migrate --dry-run
# or
rookify migrate -d

that way you can run rookify without arguments and still get --help per default.
If you would want to truly migrate, you would run:

rookify migrate

@boekhorstb1
Copy link
Contributor Author

boekhorstb1 commented Sep 30, 2024

Alright, we came to the following decision:

Going to use --dry-run as default, when no args given

reason: less is more

@boekhorstb1
Copy link
Contributor Author

Looks now like this:

 .venv/bin/rookify --help
usage: Rookify [-h] [-d] [-s] [-m]

options:
  -h, --help         show this help message and exit
  -d, --dry-run
  -s, --show-states  Show states of the modules.
  -m, --migrate      Run the migration.

Signed-off-by: Tobias Wolf <[email protected]>
Copy link
Contributor

@NotTheEvilOne NotTheEvilOne left a comment

Choose a reason for hiding this comment

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

LGTM

@NotTheEvilOne NotTheEvilOne merged commit 225b1f7 into main Oct 1, 2024
3 checks passed
@NotTheEvilOne NotTheEvilOne deleted the prs/add-run branch October 1, 2024 07:06
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.

Add run or execute argument
2 participants