Expand environnement variable in option_manager_mixin #4812
Conversation
I added @JoshMayberry 's solution for issue 3839. pylint-dev#3839
Pull Request Test Coverage Report for Build 1121196534
💛 - Coveralls |
I'm sorry, I don't understand. This is the first time I've done anything on Github. |
|
Yes, sorry I meant add an automated test to the test suite. I just realized that it would be nice to add yourself to the contributors and to add an entry to the changelog too as this change is something that benefit from being known. |
I added a test to see if pylint handles environment variables in config file locations. Added it to test_config because it also deals with config files. To test I added an environment variable containing tmp_path and then unpacked it with os.path.expandvars.
for more information, see https://pre-commit.ci
|
Added a test to test_config, the change to the changelog and myself to the contributors. |
| Path(os.path.expandvars(os.path.expanduser("${tmp_path_env}"))) / ".pylintrc" | ||
| ) | ||
|
|
||
| config_file.write_text( |
There was a problem hiding this comment.
Shouldn't we call the function read_config_file in order to test what we want to test ?
tests/test_config.py
Outdated
| # if it has an environment variable. | ||
| os.environ["tmp_path_env"] = str(tmp_path) | ||
| config_file = ( | ||
| Path(os.path.expandvars(os.path.expanduser("${tmp_path_env}"))) / ".pylintrc" |
There was a problem hiding this comment.
I think we should not expand the value in the test, to be able to see that the extends is properly done inside the function ? What am I missing ?
Tmp_path gets converted to an environment variable which is fed to the read_config_file function to be tested. Read_config_file succesfully reads the environment variable, if the variable is changed, the function raises an OSError.
for more information, see https://pre-commit.ci
|
No, you are quite right, Pierre. Thank you for your feedback. |
| os.environ["tmp_path_env"] = str(tmp_path / "pylintrc") | ||
| options_manager_mix_in = OptionsManagerMixIn("", "${tmp_path_env}") | ||
| options_manager_mix_in.read_config_file("${tmp_path_env}") | ||
|
|
There was a problem hiding this comment.
Thank you for the change :) I think we should also check that jobs = 10 or reports = yes so we make sure that the file was read correctly :)
There was a problem hiding this comment.
Love the dedication!
I'll see what I can do.
There was a problem hiding this comment.
I couldn't figure out how to get a provider to set _all_options[opt].
Is there a UML diagram of pylint somewhere?
So I just checked through the parser in load_config_file().
There was a problem hiding this comment.
Pyliny has pyreverse in it that could generate the uml diagram automatically 😄 Thank you for adding the test and the assert. Looking at the MR it seems you're testing the value in the config file and this could be merged ?
There was a problem hiding this comment.
I don't know what the MR is, but yes, the class instance reads the config file and then loads it and I used the parser that is then created to check if the values are the same. So I think the test now tests what it has to.
Thanks for your feedback btw, I learned a lot.
Added check to see if the config file is read correctly.
for more information, see https://pre-commit.ci
|
One last thing, I know it comes from a good intention to credit the user but do not tag someone in a very high activity repo's commit ( |
Pierre-Sassoulas
changed the title
I added @JoshMayberry 's solution for issue 3839.
#3839
If a config file is provided that has an environment variable, such as "%AppData%", it fails.
This can be fixed by changing this line from config_file = os.path.expanduser(config_file) to config_file = os.path.expandvars(os.path.expanduser(config_file))
Type of Changes
Description
Closes #3839