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

Pyroma Ignores Path Ignore Patterns #106

Closed
wants to merge 1 commit into from
Closed

Pyroma Ignores Path Ignore Patterns #106

wants to merge 1 commit into from

Conversation

jayclassless
Copy link
Contributor

You explicitly use include_ignored=True when getting the file paths for the pyroma tool, but I'm wondering why?

With this argument removed, the tool finds setup.py at the root of the project, and processes it as it should.

With this argument in place, it complains about other setup.py files that I have explicitly ignored in my profile. (E.g., Some of my projects have a "demo" directory that includes other small packages with their own setup.py. I've added the demo directory to my ignore-patterns, but pyroma insists on yelling at me about them.)

@coveralls
Copy link

Coverage Status

Coverage remained the same at 78.14% when pulling 772d32b on jayclassless:pyroma_ignores_ignores into f3df68f on landscapeio:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 78.14% when pulling 772d32b on jayclassless:pyroma_ignores_ignores into f3df68f on landscapeio:master.

@carlio
Copy link
Member

carlio commented Feb 10, 2015

The idea was that by default prospector will ignore setup.py, so for pyroma to work, it had to explicitly avoid the ignoring directives.

This is a bit of a problem because tools cannot influence the profile configuration in this situation. I think that #26 might help.

Perhaps the immediate solution is to only allow a setup.py relative to the current working directory of prospector, in order to avoid examples being picked up, but on the other hand, people have used prospector on repositories containing multiple separate libraries.

@jayclassless
Copy link
Contributor Author

Ah, I base my profiles off of veryhigh, which doesn't have the ignore for setup.py, so I guess that's why this change wasn't a problem for me.

What's the reason for having prospector ignore setup.py in any case? It's valid code that should be just as neat and tidy as everything else. :)

If we don't ignore it by default, then pyroma can operate just like the others. If someone decides to explicitly hide their setup.py, then they made the choice to forgo anything pyroma may have found.

@carlio
Copy link
Member

carlio commented Feb 10, 2015

The initial logic was that not everyone cares about complete purity in their codebase. At least for me, I don't mind the occaisonal hack or workaround in test code, for example, and given the utter mess that is python packaging, I felt that setup.py fell into that catagory where making it really clean and efficient was probably not worthwhile given the constraints it was under.

This is of course just my opinion, and prospector is extremely opinionated; hopefully profiles allow for people to use their own opinions instead!

Another problem (which requires a better solution than simply ignoring the file) is that prospector warns about several things such as 'no such import' from distutils, so the setup.py file generally made noise (4 messages).

@carlio
Copy link
Member

carlio commented Feb 10, 2015

(Also you may be interested in the new strictness_none.yaml profile)

@jayclassless
Copy link
Contributor Author

Pylint is definitely a finicky jerk when it comes to imports. My latest battle with it involves namespaced packages that are more than one level deep. It is quite unhappy.

My setup.py files generally pass just fine, but I don't bother with distutils anymore, so I don't run into those errors. The PYPA folks have thrown their weight behind setuptools for the time being, and given the prevalence of pip, it's pretty much always available.

Maybe in addition to the globally-ignored files, we could build support into prospector for per-tool ignores/includes. That way we could rig the tools in whatever configuration we need (e.g., pylint could ignore setup.py, but allow other tools to see it -- or globally ignore setup.py, but pyroma could force an include of it).

@landscape-bot
Copy link

Code Health
Repository health decreased by 0.12% when pulling 772d32b on jayclassless:pyroma_ignores_ignores into f3df68f on landscapeio:master.

1 similar comment
@landscape-bot
Copy link

Code Health
Repository health decreased by 0.12% when pulling 772d32b on jayclassless:pyroma_ignores_ignores into f3df68f on landscapeio:master.

@cclauss
Copy link
Contributor

cclauss commented Aug 28, 2021

Please rebase.

@carlio carlio added the file-finder-rewrite The plan to replace the file finder in prospector should include this bug label Mar 1, 2022
carlio added a commit that referenced this pull request Mar 13, 2022
…rride the default ignored files; also added a bunch more testing for file ignoring
@carlio
Copy link
Member

carlio commented Mar 13, 2022

This is finally fixed in version 1.8.0 rc0 :-)

@carlio carlio closed this Mar 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
file-finder-rewrite The plan to replace the file finder in prospector should include this bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants