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

fix: explicitly load docparams pylint plugin in .pylintrc #109

Conversation

tk-woven
Copy link
Collaborator

@tk-woven tk-woven commented Aug 4, 2022

Description

This is toward unblocking #101 which is stuck on a pylint configuration issue. We provide some configuration in .pylintrc that seems to expect the check_docs plugin, but the new plugin is docparams. We therefore explicitly load the docparams plugin when running pylint.

Reproduction

Before this change, run

pylint setup.py

in the root of the repo, and you'll see

root@hostname:/home/dgp# pylint setup.py
************* Module /home/dgp/.pylintrc
.pylintrc:1: [E0015(unrecognized-option), ] Unrecognized option found: accept-no-param-doc, accept-no-return-doc, accept-no-yields-doc

After this change, the above example runs without errors.


This change is Reviewable

The old plugin name seems to be `check_docs`, but that plugin does not
seem to be available anymore.
Copy link
Collaborator

@kuanleetri kuanleetri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @chrisochoatri and @tk-woven)


.pylintrc line 3 at r1 (raw file):

# Copyright 2019-2020 Toyota Research Institute.  All rights reserved.
[MAIN]
load-plugins=pylint.extensions.docparams

Can we involve all the criteria under [MASTER]?

Copy link
Collaborator Author

@tk-woven tk-woven left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @chrisochoatri and @kuanleetri)


.pylintrc line 3 at r1 (raw file):

Previously, kuanleetri (Kuan-Hui Lee) wrote…

Can we involve all the criteria under [MASTER]?

Unfortunately I don't think we can. The pylint docs ask us to use MAIN. I just tried putting this in MASTER locally, but then the unrecognized-option error is raised again.

Copy link
Collaborator

@wadimkehl wadimkehl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @chrisochoatri and @kuanleetri)


.pylintrc line 3 at r1 (raw file):

Can we involve all the criteria under [MASTER]?
SGTM

Copy link
Collaborator Author

@tk-woven tk-woven left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @chrisochoatri and @kuanleetri)


.pylintrc line 3 at r1 (raw file):

Previously, wadimkehl (Wadim Kehl) wrote…

Can we involve all the criteria under [MASTER]?
SGTM

After discussing review criteria with Kuan in a Slack DM, I'll dismiss this comment and proceed to merge to unblock the MRs stuck on this. As noted, I tested the recommendation for MASTER and had no luck unfortunately, but following the instruction of the docs I linked to did the trick.

@tk-woven tk-woven dismissed kuanleetri’s stale review August 4, 2022 23:01

Investigated suggestion but it unfortuantely did not fix the problem; need to merge this to unblock other MRs.

@tk-woven tk-woven merged commit e01e18f into TRI-ML:master Aug 4, 2022
@nehalmamgain
Copy link
Contributor

Thanks so much Tyler, much appreciated!

@tk-woven tk-woven deleted the fix/tyler-kowallis/explicitly-use-docparams-pylint-plugin branch September 6, 2022 00:21
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

Successfully merging this pull request may close these issues.

4 participants