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

[linter] reduce pylint to just one command #15115

Merged
merged 4 commits into from
Apr 18, 2023

Conversation

technic
Copy link
Contributor

@technic technic commented Jan 4, 2023

We can make linter rules for conanfiles simpler by checking folder where conanfile.py is located from the pylint checker.
Then same rule can be applied both to test_package and regular recipe.


@CLAassistant
Copy link

CLAassistant commented Jan 4, 2023

CLA assistant check
All committers have signed the CLA.

@conan-center-bot

This comment has been minimized.

@prince-chrismc
Copy link
Contributor

Is the intention here to remove

enable=conan-test-no-name,
?

It seems odd right now the PR in duplicating some of the other code 😳 but this is very very interesting

@technic
Copy link
Contributor Author

technic commented Jan 9, 2023

Is the intention here to remove

enable=conan-test-no-name,

I didn't notice difference between test_package and regular recipe pylintrc except conan-missing-name and conan-test-no-name. The intention is to have one pylintrc file and don't use that enable/disable lines which you have mentioned.

If you like this change then pylintrc rules and ci scripts can be adjusted.

@prince-chrismc
Copy link
Contributor

I think it's really good, there's discussion internally about the linter being a pain point for contributing so I think this will be well received

Make sure to comment in #4 :)

@prince-chrismc prince-chrismc changed the title determine test_package folder in the lint rule [linter] reduce pylint to just one command Jan 9, 2023
@conan-center-bot
Copy link
Collaborator

Conan v1 pipeline

Sorry, the build is only launched for Access Request users. You can request access writing in this issue.


Conan v2 pipeline (informative, not required for merge)

Sorry, the build is only launched for Access Request users. You can request access writing in this issue.

@AbrilRBS
Copy link
Member

AbrilRBS commented Jan 27, 2023

This looks great, thanks for your work, we really appreciate it :) (Sorry about the delay in getting this reviewed, we're hard at work for the 2.0 launch)

Just leaving myself a reminder that as #15177 aims to implement pre-commit hooks, it would need to be updated to support this new syntax.

Also could you please update the docs regarding these commands so that they reflect the unified call?

@ericLemanissier
Copy link
Contributor

@technic can you please merge current master into your master, in order to get the fix to pylint workflow 448983c ?

@technic
Copy link
Contributor Author

technic commented Feb 20, 2023

rebased

@AbrilRBS
Copy link
Member

Thanks, we'll look into this as soon as we can!
We are currently super busy with the imminent 2.0 release so expect this PR to be delayed a bit :)

@stale
Copy link

stale bot commented Mar 23, 2023

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Mar 23, 2023
@AbrilRBS
Copy link
Member

NOT stale, just in the backlog (Sorry @technic to keep you waiting! We really appreciate your patience)

@stale stale bot removed the stale label Mar 23, 2023
@prince-chrismc
Copy link
Contributor

@technic, we finally agreed to move forward with this change but we need to update the GitHub Actions and delete the dedicated test_package linter since those will be obsolete.

I will push some commit to help ensure this gets moving again :)

@prince-chrismc
Copy link
Contributor

prince-chrismc commented Apr 17, 2023

🤔 Not sure if these are supposed to be reported

image

EDIT:

No they are not

image

@prince-chrismc
Copy link
Contributor

That matches better to https://github.com/conan-io/conan-center-index/actions/runs/4165393769 with was the last PR

@conan-center-bot conan-center-bot merged commit 137297b into conan-io:master Apr 18, 2023
@SpaceIm
Copy link
Contributor

SpaceIm commented Apr 18, 2023

Is this PR responsible of this error https://github.com/conan-io/conan-center-index/actions/runs/4736184559/jobs/8407382514?pr=17062? Seems to be a regression.

@prince-chrismc
Copy link
Contributor

Yes... Someone added checks but only in 1 of the two spots 😵‍💫 alrighty then!

@prince-chrismc
Copy link
Contributor

#17103 :)

franramirez688 pushed a commit to franramirez688/conan-center-index that referenced this pull request May 10, 2023
* determine test_package folder in the lint rule

* delete old test specific linter config + update github workflow

* fixup: put bad specific config

---------

Co-authored-by: Chris Mc <christopherm@jfrog.com>
Co-authored-by: Chris Mc <prince.chrismc@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants