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

ruff: bye-bye pylint #4732

Merged
merged 6 commits into from Feb 18, 2023
Merged

ruff: bye-bye pylint #4732

merged 6 commits into from Feb 18, 2023

Conversation

sriram-mv
Copy link
Contributor

Which issue(s) does this change fix?

N/A

Why is this change necessary?

Get developer time back, ruff is insanely fast. Takes less than a second to run compared to pylint which is in the order of minutes.

What side effects does this change have?

N/A

Mandatory Checklist

PRs will only be reviewed after checklist is complete

  • Add input/output type hints to new functions/methods
  • Write design document if needed (Do I need to write a design document?)
  • Write/update unit tests
  • Write/update integration tests
  • Write/update functional tests if needed
  • 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.

@sriram-mv sriram-mv requested a review from a team as a code owner February 17, 2023 17:58
@@ -2,6 +2,14 @@
requires = ["setuptools", "wheel"] # PEP 508 specifications.


[tool.ruff]
Copy link
Contributor

Choose a reason for hiding this comment

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

You may want to add https://github.com/aws/aws-lambda-builders/blob/develop/pyproject.toml#L4 here too. Pylint is off by default.

Copy link
Contributor Author

@sriram-mv sriram-mv Feb 17, 2023

Choose a reason for hiding this comment

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

Good to know! Ah, now there are errors to fix :)

Copy link
Contributor

Choose a reason for hiding this comment

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

😎 sam-t is away ahead of cli regarding how many rules it comply: https://github.com/aws/serverless-application-model/blob/develop/ruff.toml#L4

@@ -1 +1 @@
pylint>=2.13.0,<3
ruff
Copy link
Contributor

Choose a reason for hiding this comment

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

Ruff is not very stable and keeps adding rules. We should try to ping the version otherwise once it adds a new rule the build might fail

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any build failures you have experienced? I'm gonna pin to 0.0.247 in that case.

Copy link
Contributor

Choose a reason for hiding this comment

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

they added too-many-arguments in the past few weeks and when we upgrade there were many violations in sam-t codebase

@@ -83,6 +83,7 @@ param (
)

function Init {
pip install -e '.[pre-dev]'
Copy link
Contributor

Choose a reason for hiding this comment

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

curious about this change. I think our Makefile doesn't install the pre-dev ones:

SAM_CLI_DEV=1 pip install -e '.[dev]'

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 believe we have as many windows devs, so this might have gotten missed out, so I brought back the pre-dev step in there.

Comment on lines +22 to +23
"integration_uri.py" = ["E501"] # ARNs are long.
"app.py" = ["E501"] # Doc links are long.
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like we should probably do per-line ignores if possible, rather than the whole file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ack. its just those files that presented it here, so went with those files.

@sriram-mv sriram-mv enabled auto-merge (squash) February 18, 2023 01:20
@sriram-mv sriram-mv merged commit 93462ee into aws:develop Feb 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants