Skip to content
This repository has been archived by the owner on Sep 27, 2024. It is now read-only.

Basic linter config #261

Merged
merged 19 commits into from
Apr 1, 2023
Merged

Basic linter config #261

merged 19 commits into from
Apr 1, 2023

Conversation

hanneshapke
Copy link
Contributor

@hanneshapke hanneshapke commented Jan 16, 2023

What does this pull request do?

This PR set up a basic linter configuration for the MCT repo. It is a replacement for the closed PR: #242

The pylintrc is mimic'ing Google's pylintrc: https://github.com/google/styleguide/blob/gh-pages/pylintrc
with two additional disabled checks (due to quantity):

  • abstract-class-instantiated
  • protected-access

All other initial issues have been addressed to make the code base compliant with the pylintrc:

  • unused imports
  • super-with-arguments
  • inconsistent-quotes
  • unneeded not
  • line-too_long

Fixes #246

How did you test this change?

No functionality of tests changes - only formatting and styling of the code itself.

How did you document this change?

CONTRIBUTING.md has been updated

Before submitting

Before submitting a pull request, please be sure to do the following:

  • Read the How to Contribute guide if this is your first contribution.
  • Open an issue or discussion topic to discuss this change.
  • Write new tests if applicable.
  • Update documentation if applicable.

@hanneshapke hanneshapke marked this pull request as draft January 16, 2023 22:49
@hanneshapke hanneshapke marked this pull request as ready for review January 17, 2023 03:18
@codesue
Copy link
Contributor

codesue commented Jan 18, 2023

Hi @hanneshapke, I'm not feeling well and might not be able to take a look at your PR this week, so I requested additional reviewers.

@hanneshapke
Copy link
Contributor Author

Thank you, @codesue. Get well soon!

.github/workflows/lint.yml Outdated Show resolved Hide resolved
elif isinstance(subfield_json_value, list):
subfield_value = []
for item in subfield_json_value:
if isinstance(item, dict):
new_object = field.__annotations__[subfield_key].__args__[0]() # pytype: disable=attribute-error
new_object = \
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we disable line length here? This is hard to read.

@@ -207,7 +205,8 @@ def _annotate_eval_results(self, model_card: ModelCard) -> ModelCard:
output_file_format=self._source.tfma.file_format)
if eval_result:
logging.info('EvalResult found at path %s', eval_result_path)
if self._source.tfma.metrics_include or self._source.tfma.metrics_exclude:
if self._source.tfma.metrics_include or \
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we use parentheses instead of \ here and elsewhere?

model_card_toolkit/utils/validation_test.py Show resolved Hide resolved
"output_dir",
default="/tmp/model_card_toolkit",
help="Where to output the docs")
flags.DEFINE_string("output_dir",
Copy link
Contributor

Choose a reason for hiding this comment

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

I personally find the original way of putting the arguments on a new line easier to read. I'm not familiar enough with yapf to know exactly which rule controls this. Maybe SPLIT_BEFORE_EXPRESSION_AFTER_OPENING_PAREN or SPLIT_BEFORE_FIRST_ARGUMENT?

.github/workflows/lint.yml Show resolved Hide resolved
@hanneshapke
Copy link
Contributor Author

Passing linter run: https://github.com/tensorflow/model-card-toolkit/actions/runs/4520740897
Updated the line breaks after parens

@hanneshapke hanneshapke merged commit b3baa1f into tensorflow:main Apr 1, 2023
@hanneshapke hanneshapke deleted the linter-setup-v2 branch April 1, 2023 21:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Set up linting pre-commit hook
2 participants