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

Needs a mechanism for specifying more nuanced strictness #9

Closed
ssokolow opened this issue Jan 14, 2014 · 10 comments
Closed

Needs a mechanism for specifying more nuanced strictness #9

ssokolow opened this issue Jan 14, 2014 · 10 comments

Comments

@ssokolow
Copy link

Prospector and Landscape.io need some way to say "I want 'veryhigh' strictness in general, but use 'medium' for PyLint and use this raw config file with pep8.py instead."

prospector -s medium doesn't enforce PEP8 strictly enough but prospector -s high makes PyLint too pedantic and makes PEP8 complain about "E401: multiple imports on one line", which I've ignored in my flake8 configuration.

As-is, I have to continue to enforce my PEP8 policy by leaving Travis-CI configured to run flake8 and fail the entire test run if the code fails the style check.

For pep8.py, might I suggest just restoring its ability to let a [pep8] section in setup.cfg or tox.ini override the default values? (In other words, supply the prospector configuration via whatever mechanism pep8 --config uses and make sure Prospector isn't confusing its idea of where the project's root directory is.)

@carlio
Copy link
Member

carlio commented Jan 14, 2014

Two things to mention here:

Firstly, there's the (totally undocumented) profiles support. This allows you to specify exactly which errors and options to use for the underying tools, by creating a YAML file. It includes inheritance, so you can inherit from the 'strictness_medium' profile for example. It's not documented because it's still a work in progress; the "strictness" is really just a special case of it, and I don't want at this early stage to commit to exactly what can and can't be done with profiles. I like the idea of people publishing their "profiles" as a representation of their coding style, and I'd love to see people comparing, borrowing or just discussing the "profiles", but that's a way off! Have a look at https://github.com/landscapeio/prospector/tree/master/prospector/profiles/profiles if you're interested in the nitty-gritty though.

Secondly, I do want to include an ability to delegate config or something like that. I don't think it's prospector's job to be able to control all the nuances of every sub-tool - if your opinions are so strong about each tool, it's probably better to dig into it yourrself. However it'd be nice to use existing config in an easy way. I'd rather that people who have strong opinions can have prospector collect them, rather than have prospector allow you to define your strong opinions.

@carlio
Copy link
Member

carlio commented Jan 14, 2014

Also I realise that my comment didn't answer the actual question about strictness per-tool.

Profiles were designed from the start to be combinable; it should not be hard to create per-tool profiles which can be combined. In fact, that's what already happens with the '--doc-warnings' flag and similar. The question is how much of this should be baked into the prospector command line, and how.

@carlio
Copy link
Member

carlio commented Jan 14, 2014

Thinking about it a little more, the profiles already have separate sections per tool. Perhaps segregating these would be the the answer, so that

prospector -s medium
is actually shorthand for

prospector --pep8-strictness-medium --pylint-strictness-medium --pyflakes-strictness-medium

I don't like this particularly though, it smacks of abstraction leak. I think the real issue is that PEP8 compliance is a different beast compared to the other tools, who aim to point out logic or structure flaws rather than stylistic flaws.

@ssokolow
Copy link
Author

Firstly, there's the (totally undocumented) profiles support.

I'm glad to hear that but is it possible yet for a project to specify a custom profile in a manner Landscape.io will obey?

Secondly, I do want to include an ability to delegate config or something like that. I don't think it's prospector's job to be able to control all the nuances of every sub-tool - if your opinions are so strong about each tool, it's probably better to dig into it yourrself. However it'd be nice to use existing config in an easy way. I'd rather that people who have strong opinions can have prospector collect them, rather than have prospector allow you to define your strong opinions.

The reason I suggested feeding the Prospector-supplied config into pep8.py via the mechanism which powers --config is that you're already supporting PyLint pragma comments and pep8.py's --config option sets up a similar relationship between the specified config file and the project's setup.cfg and/or tox.ini files.

pep8.py would use the Prospector-supplied config as a base, but then overlay the settings specified using a [pep8] section in setup.cfg and/or tox.ini.

I don't like this particularly though, it smacks of abstraction leak. I think the real issue is that PEP8 compliance is a different beast compared to the other tools, who aim to point out logic or structure flaws rather than stylistic flaws.

But PyLint tries to be a style checker on the higher levels of strictness too. (In fact, I still have to add ignore lines because, on high strictness, PyLint assumes every module-level variable will be a constant and complains if they're not named using all caps.)

PyFlakes and PyChecker are the only tools I've seen which limit themselves to logic and structure flaws.

carlio added a commit that referenced this issue Jan 25, 2014
…ictness. This also introduces "enabling" messages in profiles, essentially cancelling out a previous "disable"
carlio added a commit that referenced this issue Jan 25, 2014
@carlio
Copy link
Member

carlio commented Jan 25, 2014

The above changes add a new flag -F or --full-pep8 which will turn on complete PEP8 compliance checking regardless of strictness. This doesn't completely solve the issue of nuanced strictness, but should help out your particular use case. I'll continue to investigate better ways to configure the underlying tools as well.

@ssokolow
Copy link
Author

Not helpful at all unless Prospector also allows pep8.py to obey my ignore = E126,E221,E302,E401 line.

As I said in my original post, full PEP8 compliance involves complaining about things like import os, sys which I refuse to give up. (I hate unnecessary stylistic choices which waste screen space with lines that are pointlessly short, like putting the opening curly brace on its own line in C code or doing one import per line in Python.)

You still haven't said what's wrong with calling pep8.py in a manner equivalent to pep8.py --config prospector_profile.ini so that a project's setup.cfg and/or tox.ini can override settings specified by Prospector. (In much the same way that PyLint pragmas already can on a line-by-line basis.)

@carlio
Copy link
Member

carlio commented Jan 25, 2014

There's nothing wrong with it, and I'm looking into how to get it working now. Prospector calls pep8 directly rather than running it as a subprocess, so it's just a case of figuring out how to programatically specify the config file, and then figure out how to merge or override PEP8 config which comes from the strictness profiles.

The behaviour should be, I think: "use tox.ini or setup.cfg if present, else fall back to the prospector defaults". That goes for all tools as well, including .pylintrc for example.

@ssokolow
Copy link
Author

Ahh. Makes sense.

If I'm reading the source of pep8.py and flake8/engine.py correctly, --config is set internally by passing config_file as an argument when instantiating StyleGuide.

(Flake8 resets the user defaults file from ~/.config/pep8 to ~/.config/flake8)

Also, here's my vote for merging rather than overriding if at all possible. (And if it's not possible, prospector should probably have an option to dump its default config a given tool to the location the tool will check so you can just edit it.)

carlio added a commit that referenced this issue Jan 26, 2014
…nd, and fall back on Prospector-provided defaults
@carlio
Copy link
Member

carlio commented Jan 26, 2014

The PEP8 tool now uses values in tox.ini, setup.cfg, .pep8 in the root folder (ie the argument to --path), and also ~/.config/pep8. The semantics are "override" but actually it turns out "merging" is easier than I thought.

Probably a new argument would make sense:

  • --external-config=none to force using Prospector's values
  • --external-config=merge to merge existing and Prospector's values
  • --external-config=only to use only existing config

carlio added a commit that referenced this issue Jan 26, 2014
…ack to prospector values if no external config exists. The tool also respects the command line switch to override default behaviour, and will merge external config with prospector default config if --external-config merge is used
@carlio
Copy link
Member

carlio commented Jan 26, 2014

You should be able to use your existing config in combination with prospector now using prospector -e merge - let me know if it doesn't work as expected!

Note that .pylintrc files are not honoured, but that will be dealt with by #10

@carlio carlio closed this as completed Jan 26, 2014
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

No branches or pull requests

2 participants