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

feat: fix latent pylint errors in many source files #115

Merged

Conversation

tk-woven
Copy link
Collaborator

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

Description

This is toward #114. We'd like to replace the many-entrypoint linting with the single-entrypoint pre-commit tool. When run in CI, we'll run pre-commit against all files for convenience. So, I ran pylint against all files in DGP and found some latent issues in master. This MR addresses the issues. The issues are almost entirely documentation issues, so the bulk of this MR is docs changes.

Note one quirk. According to the numpydoc style guide, we should be able to document parameters as

Parameters
----------
name: type, default: value
    Description.

if name gets a default value such as in f(name: str = "asdf"). However, pylint with the numpydoc plugin does not seem to recognize the default: value syntax when placed inline with the type documentation. To get around this, we use the canonical optional annotation and move the default value annotation into the parameter description.

Besides these docs changes, this does introduce some new code for calls to open. Pylint is very strict and requires we provide an encoding, so we just explicitly specify what will be used as the default in the end anyway, locale.getpreferredencoding(). Alternatively, we could suppress this pylint warning. This is only done for text-mode (not binary-mode) files.

Lastly, there were a few places where I simply couldn't intuit the types reasonably. In these cases (and in test code), I simply asked pylint to ignore missing docs.

Checks

I built the docs locally after this change. There are very many Sphinx warnings about unexpected section titles (for all sections: Parameters, Returns, etc.) in documentation of callables. These issues appear to exist on master already.

I reviewed the built documentation (and PDF, a la rinoh), and they still appear conveniently human-readable to me.


This change is Reviewable

@tk-woven tk-woven force-pushed the fix/tyler-kowallis/fix-latent-pylint-issues branch from 4f6607c to 953e53f Compare August 12, 2022 01:35
@tk-woven tk-woven marked this pull request as ready for review August 12, 2022 01:41
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.

Wow, that's massive! Great effort :) :lgtm:

Reviewed 39 of 39 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @chrisochoatri and @kuanleetri)

@tk-woven
Copy link
Collaborator Author

Thanks!

@tk-woven tk-woven merged commit 2dea870 into TRI-ML:master Aug 12, 2022
@tk-woven tk-woven deleted the fix/tyler-kowallis/fix-latent-pylint-issues 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.

2 participants