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

[ECS task] Run pre commit argument switch #652

Closed

Conversation

jmatthiesen81
Copy link

Q A
Branch master for features and deprecations
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Documented? no
Fixed tickets none

This pull request extends the task with the configuration files_on_pre_commit to check the staged files on the pre commit instead of the files in the directories configured in whitelist_patterns. To keep the backward compatibility the configuration is disabled in the default.

@jmatthiesen81 jmatthiesen81 changed the title Run pre commit argument switch [ECS task] Run pre commit argument switch Jul 5, 2019
Copy link
Contributor

@veewee veewee left a comment

Choose a reason for hiding this comment

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

Thanks for the PR.
I've added a remark to the code.

Can you also:

  • rebase against master (to fix appveyor build)
  • add documentation
  • add spec tests for the new configurable option
  • add spec tests for the new functionality

src/Task/Ecs.php Outdated
$arguments->addFiles($files);
}

if (!$config['files_on_pre_commit'] && $context instanceof RunContext) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This statement doesn't seem right: in run context it should always use the whitelist instead of the files?

@jmatthiesen81 jmatthiesen81 force-pushed the run-pre-commit-argument-switch branch from 096cb00 to daf0557 Compare October 30, 2019 13:55
@jmatthiesen81 jmatthiesen81 force-pushed the run-pre-commit-argument-switch branch from daf0557 to cbcfab2 Compare December 18, 2019 14:20
@veewee
Copy link
Contributor

veewee commented Aug 27, 2020

Thanks for the idea!
Added this to #809 and added docs + tests!

@veewee veewee closed this Aug 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants