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

User configurable config file location #3191

Closed

Conversation

cdayjr
Copy link

@cdayjr cdayjr commented Jan 2, 2021

This fixes #2062 in a hopefully backwards-compatible way.

Tests were failing due to the new year so I did a quick update for those as well.

I'll gladly add tests / update documentation if I can be pointed in the right direction on those.

@jrfnl
Copy link
Contributor

jrfnl commented Jan 2, 2021

Tests were failing due to the new year so I did a quick update for those as well.

Just had a quick look and it seems like you've (incorrectly) changed the copyright of some files which are unrelated to the test failure as well. Please undo those changes.

The only files which need changing are the two .fixed files. See https://github.com/squizlabs/PHP_CodeSniffer/pull/2793/files for last years commit.

I can't say much about the actual change proposed here.

I do wonder what the use-case is for this change as the problem described in #2062 can easily be solved by using the Composer PHPCS plugin to set the paths or - per one of the replies in the thread - by using a post-install/update-cmd script in the composer.json.

Note: if this change would be accepted, the authors of the Composer PHPCS plugin ought to be notified as that plugin will need to be adjusted to take this new environment variable - which may or may not exist - into account as well.

@cdayjr
Copy link
Author

cdayjr commented Jan 2, 2021

@jrfnl

Tests were failing due to the new year so I did a quick update for those as well.

Just had a quick look and it seems like you've (incorrectly) changed the copyright of some files which are unrelated to the test failure as well. Please undo those changes.

The only files which need changing are the two .fixed files. See https://github.com/squizlabs/PHP_CodeSniffer/pull/2793/files for last years commit.

Sure thing, 9e7645a should address that.

I can't say much about the actual change proposed here.

I do wonder what the use-case is for this change as the problem described in #2062 can easily be solved by using the Composer PHPCS plugin to set the paths or - per one of the replies in the thread - by using a post-install/update-cmd script in the composer.json.

Note: if this change would be accepted, the authors of the Composer PHPCS plugin ought to be notified as that plugin will need to be adjusted to take this new environment variable - which may or may not exist - into account as well.

I checked out that plugin and it only covers setting installed_paths. Since running composer update wipes the configuration file every time if you have phpcs installed via composer, other configuration settings would still get deleted even if you're using that plugin. Even if the issue was resolved by that plugin I'd imagine it's a problem you'd want solved in the tool itself though; I'm not using that plugin and I'm sure I'm not the only one. I was able to work around this issue by having composer update tied to a script that copies over the configuration file from a backup whenever its ran and it seems like others have similar solutions to this in that issue but weird workarounds like that aren't really ideal.

I also checked how the plugin was setting the installed_paths:

https://github.com/Dealerdirect/phpcodesniffer-composer-installer/blob/c4a47f7f87efa81b0e65470793ca6e2cb798eb07/src/Plugin.php#L268-L285

It looks like that plugin relies on the --config-set and --config-delete to set configuration values and doesn't make assumptions about where the CodeSniffer.conf file is, so this change would not affect the functionality there.

Additionally, if someone does need to modify the file directly for whatever reason, phpcs --config-show does print out which file it's using at the top already, e.g.:

Using config file: /home/cdayjr/.config/composer/vendor/squizlabs/php_codesniffer/CodeSniffer.conf
...

@cdayjr
Copy link
Author

cdayjr commented Mar 27, 2021

What's the status on this? If there are any changes required I'd be happy to make them and get this change out there.

@cdayjr cdayjr closed this Oct 3, 2024
@cdayjr cdayjr deleted the user-configurable-config-file-location branch October 3, 2024 20:53
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.

Allow CodeSniffer.conf to be stored in a user's home directory
2 participants