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

[919] Add logic to scan the current directory and add any files that … #920

Closed

Conversation

jklmnop
Copy link

@jklmnop jklmnop commented Aug 6, 2021

Q A
Branch master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Documented? yes
Fixed tickets #919

Unfortunately, this will not work until friendsoftwig/twigcs changes how they exclude files, but there is a PR for it.

see: friendsoftwig/twigcs#183
see: friendsoftwig/twigcs#184

This bit of code will compare the files that have changed with the files in the project and add non-matching file paths to the exclude config.

New Task Checklist:

  • Are the dependencies added to the composer.json suggestions?
  • Is the doc/tasks.md file updated?
  • Are the task parameters documented?
  • Is the task registered in the tasks.yml file?
  • Does the task contains phpunit tests?
  • Is the configuration having logical allowed types?
  • Does the task run in the correct context?
  • Is the run() method readable?
  • Is the run() method using the configuration correctly?
  • Are all CI services returning green?

…haven't been changed to the exclude list.

Requires a patch in friendsoftwig/twigcs that adds file exclusion.

@see friendsoftwig/twigcs#183
@see friendsoftwig/twigcs#184
@veewee
Copy link
Contributor

veewee commented Aug 6, 2021

Since twigcs does not support this yet, wouldn't it make sense to work with an include list instead of an exclude?
That way we could just pass the list of all changed twig files instead of adding some dark magic exclude logic :)

@jklmnop
Copy link
Author

jklmnop commented Aug 6, 2021

@veewee Absolutely, but TwigCS does not support StdIn or an "include" list at this moment, so I'm just trying to work with what they do have. 🤦‍♀️

I do not think this is the best solution for the problem, but as of now, I think it's the ONLY solution with the options they provide (and my patch to their project). I don't think it should even make it into master.

At least there is a commit someone can reference with the dependency in composer.json in the event they need this problem solved too. Failing commits for unchanged files is a huge bummer and would mean we couldn't reliably use the tool without a bunch of unnecessary noise and false positive failures and git commit -n.

I'm hoping they'll at least change the strategy for exclusion, and better yet, maybe someone much more knowledgable than me could implement a StdIn/include strategy a la PHPCS. Best I could do right now is change which method they're calling to scan for excludes in TwigCS coupled with the "dark magic 🧙‍♂️" in this commit for GrumPHP.

@veewee
Copy link
Contributor

veewee commented Sep 24, 2021

Hello @jklmnop

Thanks for investigating this.
I am closing this PR for now. Feel free to provide a new PR if TwigCs decides to support this and you got a working PR.

@veewee veewee closed this Sep 24, 2021
@jklmnop
Copy link
Author

jklmnop commented Sep 24, 2021

Thank you so much, @veewee! Eventually, I realized that a patch might be a better option since I was losing out on any updates to master without maintaining this PR. (Here's that patch for anyone who finds themselves here)

I might take a crack at making TwigCs accept a piped list of files a la PHP_CodeSniffer and other projects. Hoping if they do eventually support it, there won't be a need to update GrumPHP!

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.

None yet

2 participants