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

chore: Enable some pylint rules and add clarity to pylintrc file #2375

Merged
merged 8 commits into from
Nov 14, 2020

Conversation

aahung
Copy link
Contributor

@aahung aahung commented Nov 13, 2020

Why is this change necessary?

Currently there are multiple pylint rules we disable without clarity. It is hard to find which rules are necessarily to disable, and which are not.

How does it address the issue?

This PR exposes the rule name in pylintrc file, and (re)enabled some rules

What side effects does this change have?

absolutely no side effect

Checklist

  • Write design document (Do I need to write a design document?)
  • Write unit tests
  • Write/update functional tests
  • Write/update integration tests
  • make pr passes
  • make update-reproducible-reqs if dependencies were changed
  • Write documentation

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@aahung aahung requested review from a team and hoffa November 13, 2020 22:28
@@ -24,20 +24,20 @@ class BuildContext:
_BUILD_DIR_PERMISSIONS = 0o755

def __init__(
self,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the formatting different?

Copy link
Contributor

Choose a reason for hiding this comment

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

Well as per @aahung

I just ran black on that file, it does not do anything, then I made a small change, it reformatted it

It seems black isn't yet stable. 😩😩😩

Great PR otherwise, approved.

@@ -263,4 +264,4 @@ def decorator_customize_config_env(f):
return click.option(*config_env_param_decls, **config_env_attrs)(f)


# End section copied from [[click_config_file][https://github.com/phha/click_config_file/blob/master/click_config_file.py]
# End section copied from [click_config_file][https://github.com/phha/click_config_file/blob/master/click_config_file.py
Copy link
Contributor

Choose a reason for hiding this comment

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

[minor] will the hyperlink work with the last close bracket missed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is a python file so hyperlink won't work anyway

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't feel the original comment was trying to follow format like markdown 🤔

@aahung aahung merged commit 79d1770 into aws:develop Nov 14, 2020
sriram-mv pushed a commit that referenced this pull request Nov 23, 2020
* Revert "chore: Enable pylint compare to zero/empty string"

This reverts commit 1e3b282.

* Revert "chore: Enable pylint Else If Used checker"

This reverts commit 5819ded.

* Revert "chore: Remove unnecessary pylint disable"

This reverts commit d4a5c46.

* Revert "chore: Enable pylint line too long"

This reverts commit fb54504.

* Revert "chore: Enable pylint multi-processes"

This reverts commit b02c8d9.

* Revert "chore: Enable pylint rule Unused import"

This reverts commit a6eda1d.

* Revert "chore: Remove pylint rules that do not apply for python 3"

This reverts commit 6212005.

* Revert "chore: Make pylintrc ignore rule more readable"

This reverts commit fe50385.
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