[BUG] Version 1.7.0 backward incompatibility: pep8/pep257 config is not read anymore from prospector.yaml

[volans-]

Describe the bug
Prospector 1.7.0 introduces a backward incompatible change. The settings for pep8 and pep257 in prospector.yaml are now ignored although the release notes report that the change should be backward compatible and the existing configuration should still work:

Deprecation warning:
Tools pep8 and pep257 have been renamed to pycodestyle and pydocstyle respectively. This is because the tools themselves were renamed years ago - See #222. Note that this means that prospector profiles and message output uses this new name instead of the old name, so you will need to update your configuration. The old names will still work, but this legacy behaviour will be removed in prospector 2.0.

Renaming the keys in prospector.yaml from pep8 to pycodestyle and from pep257 to pydocstyle is not an option because it would make previous versions of prospector ignore their related configuration and hence will report the wrong issues when run.

To Reproduce
Steps to reproduce the behavior:

1. Add some pep8/pep257 specific configuration in prospector.yaml, for example:

pep8:
  full: true
  options:
    max-line-length: 120

2. Have a code file with a line length between 101 and 120 characters.
3. Run prospector up to v1.6.* and it will pass
4. Run prospector 1.7.0 or 1.7.1 and it will fail with:

pycodestyle: E501 / line too long (106 > 99 characters) (col 100)

Expected behavior
Prospector should load the pep8/pep257 specific configuration from prospector.yaml and for example in this specific case not report the line length issue.

Environment (please complete the following information):

• OS: macOS
• Tool pep8/pep257
• Prospector version 1.7.1
• Python version 3.9.10

[carlio]

Apologies @volans- : this was supposed to be backwards compatible, not breaking.

I have added tests and fixed the code so that both old and new tool names should be equivalent (until version 2.0 when the old names will be removed).

Please let me know if these work for you, it does on my repro steps and in unit tests at least.

Version 1.7.2 is on PyPI now.

[volans-]

@carlio Thanks a lot for looking into it, the prompt fix and the new release. I can confirm that the configuration is now properly loaded with the old identifier names for pep8 and pep257.

However I noticed a related issue regarding the inheritance of configurations.
In my settings I have:

inherits:
  - strictness_high

And it seems that those settings are not anymore loaded. For example for pydocstyle it should disable D400 and D401, but is not doing it anymore.
I think that this is because in prospector/profiles/profiles/strictness_high.yaml the base settings for those have been renamed to pycodestyle and pydocstyle and with your fix above those get overridden on line 79 and 86 respectively by:

profile_dict["pycodestyle"] = profile_dict["pep8"]
profile_dict["pydocstyle"] = profile_dict["pep257"]

I'm not familiar with prospector source code, but I think that replacing those 2 lines with the following 2, respectively, should fix the problem:

profile_dict["pycodestyle"] = _merge_tool_config(profile_dict["pep8"], profile_dict["pycodestyle"])
profile_dict["pydocstyle"] = _merge_tool_config(profile_dict["pep257"], profile_dict["pydocstyle"])

But I'm not sure if there are other aspects to take into account that I'm missing.

Let me know if you want me to open a separate issue or you want to reopen this one.

[carlio]

Thank for letting me know - I had forgotten to write tests to think about inheritance!

You're right, the code will overwrite pycodestyle config with any pep8 found because I was writing it only thinking about the contents of one profile file, not thinking about the inheridance.

I'll re-open this because it's still the same issue pretty much. There'll probably be a new release with that fix in too, I'm on a mission to kill bugs and already have a few more fixes I want to release anyway :-)

[carlio]

@volans- I have pushed 1.7.3 now with what I think is the correct fix. Instead of trying to merge profiles after parsing, I realised just pre-filtering the YAML contents to rename 'pep8' to 'pycodestyle' was easier :-)

The unit tests I added seem to work including with inheritance, please let me know if you see problems with it still.

(I am ever the optimist so I'll close this again but if it's still broken please just re-open!)

[volans-]

@carlio I can confirm that 1.7.3 runs cleanly on my setup as 1.6.0. Thanks a lot for the quick turnaround and final fix!

[carlio]

Cool beans!

FYI, I had to yank 1.7.2 and 1.7.3 due to other breaking regressions - 1.7.4 is now released to replace them.

[volans-]

@carlio sorry to bother you again. In another repository of mine with similar settings, with prospector 1.7.4 I get:

prospector.yaml
  Line: 20
    profile-validator: unknown-setting / "pep8" is not a valid prospector setting
  Line: 27
    profile-validator: unknown-setting / "pep257" is not a valid prospector setting

A did a quick look and this seems to be triggered by this check becaue tool_names() in https://github.com/PyCQA/prospector/blob/master/prospector/tools/profile_validator/__init__.py#L50 seems to not take into account the new DEPRECATED_TOOL_NAMES constant.

I'm not sure why on one repository this fails, while in another repository this does not fail and runs fine.
Their prospector.yaml config are very similar, the only difference I noticed is that in the one that fails there is also an ignore-paths setting, I'm not sure if that might affect the behaviour of the profile validation.

P.S. I don't have permission to re-open this issue.

[carlio]

@volans- It's not a bother at all! I'm currently trying to tidy up the repository and close the backlog of bugs, I expected there'd be some new ones appear as the unit tests only go so far. Thank you for your patience and help finding and debugging these issues!

Not sure why you can't re-open it but just create new issues if you find any more problems, that'd be easiest I guess.

Regarding the problem of 'unknown-setting' that is indeed an oversight and I have fixed it in carlio@cb274a5 which will go out in a 1.7.5 release later.

It does raise a new error though, it's a deprecation warning instead of an unknown-setting, but that's valid as the setting is indeed deprecated.

[volans-]

@carlio Perfect, thanks a lot. I'll let you know once 1.7.5 is out.

[carlio]

1.7.5 just pushed to PyPI

[volans-]

@carlio I can confirm it all works fine if I add this to prospector.yaml:

profile-validator:
  disable:
    - deprecated-tool-code

With the above prospector runs fine both on 1.6.0 and 1.7.5.

Strictly speaking this is still slightly backward incompatible as I had to change my configuration, but that's not a problem for me, just wanted to mention that.
An alternative approach could be to put a deprecation warning using the warnings module:

warnings.warn("your message", DeprecationWarning)

(see https://docs.python.org/3/library/warnings.html#warnings.warn )
But that would probably go a bit outside of the usual way of reporting of prospector.

Thanks again for all the fixes and the quick turnarounds. This can be considered closed for me.