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

[FEATURE] Introduce --dry-run option to commands #42

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

gilbertsoft
Copy link
Contributor

Resolves #40

Copy link
Member

@o-ba o-ba left a comment

Choose a reason for hiding this comment

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

Hi, I would like to keep this PR as clean as possible. Additionally, this repo is already using the rules from "typo3/coding-standards". Applying further code styles, not listed in the mentioned rules, for a couple of files only, makes further maintenance unnecessary hard. Could you therefore please revert the code style changes?

src/Command/AbstractClientRequestCommand.php Outdated Show resolved Hide resolved
@gilbertsoft gilbertsoft force-pushed the feature/dry-run-option branch 2 times, most recently from 1b901d5 to 3f2af41 Compare November 17, 2021 17:05
@gilbertsoft
Copy link
Contributor Author

Hi, I would like to keep this PR as clean as possible. Additionally, this repo is already using the rules from "typo3/coding-standards". Applying further code styles, not listed in the mentioned rules, for a couple of files only, makes further maintenance unnecessary hard. Could you therefore please revert the code style changes?

Done

@o-ba
Copy link
Member

o-ba commented Nov 19, 2021

We should also adjust the README correspondingly.

@gilbertsoft
Copy link
Contributor Author

We should also adjust the README correspondingly.

Agree but I'm currently not able to change it because there are too many problems which will get fixed automatically and deactivating the md linter extensions does not help :(

src/Command/AbstractClientRequestCommand.php Outdated Show resolved Hide resolved
->addOption('no-docs', '', InputOption::VALUE_OPTIONAL, 'Disable version update in documentation settings', false);
->addOption('no-docs', '', InputOption::VALUE_OPTIONAL, 'Disable version update in documentation settings', false)
->addOption('dry-run', null, InputOption::VALUE_NONE, 'Outputs the operations but will not execute anything')
;
Copy link
Member

Choose a reason for hiding this comment

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

Same as above.

} catch (\InvalidArgumentException $e) {
$io->error(sprintf('An error occurred while setting the ext_emconf.php version to %s.', $version));
return 1;
if (!$dryRun) {
Copy link
Member

Choose a reason for hiding this comment

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

Could we just do an early return above L:71? There is nothing to do below, in case this is a dry run. Would make the change a bit smaller.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added a note here and below. If we like to keep this output, no.

@gilbertsoft gilbertsoft marked this pull request as draft November 26, 2021 10:54
@gilbertsoft
Copy link
Contributor Author

We should also adjust the README correspondingly.

Agree but I'm currently not able to change it because there are too many problems which will get fixed automatically and deactivating the md linter extensions does not help :(

Handled with #48. Afterwards I'm able to add the dry-run option.

@@ -68,6 +70,12 @@ protected function execute(InputInterface $input, OutputInterface $output): int
->setRaw($input->getOption('raw') !== false)
->setDefaultAuthMethod($this->defaultAuthMethod);

// In case of a dry-run the request to TER is skipped.
if ($input->getOption('dry-run') === true) {
$io->note(sprintf('Would have sent the command "%s %s", skipping', $requestConfiguration->getMethod(), $requestConfiguration->getEndpoint()));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we should output the command name instead, not sure....

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.

[FEATURE] --dry-run option
3 participants