Skip to content

Use lintrunner #168

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

Merged
merged 4 commits into from
Nov 14, 2022
Merged

Use lintrunner #168

merged 4 commits into from
Nov 14, 2022

Conversation

justinchuby
Copy link
Collaborator

@justinchuby justinchuby commented Nov 9, 2022

Adopt lintrunner for running all linters locally and in CI. https://github.com/suo/lintrunner

  • Remove all formatting scripts in favor of lintrunner
  • Updated readme with new instructions
  • Run lintrunner -a to fix all lint errors
  • Support SARIF results in Github

Nice SARIF error messages:

e.g. https://github.com/microsoft/onnx-script/security/code-scanning/335

image

@codecov
Copy link

codecov bot commented Nov 9, 2022

Codecov Report

Merging #168 (d0ba635) into main (0b64a9e) will not change coverage.
The diff coverage is 90.00%.

@@           Coverage Diff           @@
##             main     #168   +/-   ##
=======================================
  Coverage   81.04%   81.04%           
=======================================
  Files          66       66           
  Lines        5133     5133           
=======================================
  Hits         4160     4160           
  Misses        973      973           
Impacted Files Coverage Δ
onnxscript/backend/onnx_backend.py 78.39% <0.00%> (ø)
onnxscript/test/models/graph_attr.py 65.11% <100.00%> (ø)
onnxscript/test/onnx_backend_test.py 74.17% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@justinchuby
Copy link
Collaborator Author

justinchuby commented Nov 9, 2022

@abock @gramalingam please take a look, thanks! I would like to make sure licensing and attribution is good for files under .lintrunner/

@titaiwangms
Copy link
Contributor

What does new friends: newlines/grep do?

@gramalingam
Copy link
Collaborator

I would like to make sure licensing and attribution is good for files under .lintrunner/

Do you mean from a legal perspective? Let me defer that to Aaron. I guess it should not be a problem.

Copy link
Contributor

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

lintrunner found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.

@abock
Copy link
Contributor

abock commented Nov 10, 2022

I would like to make sure licensing and attribution is good for files under .lintrunner/

Do you mean from a legal perspective? Let me defer that to Aaron. I guess it should not be a problem.

It's MIT licensed, so no issues here as long as the original license files, annotations, and copyrights are preserved.

However, is there a reason for including all the code in the repo? Can this be installed as a build dependency or should we submodule it?

I will do a deeper review tomorrow.

@justinchuby
Copy link
Collaborator Author

justinchuby commented Nov 10, 2022

The original adapters are part of the pytorch repo. I guess we can create a package for these adapters? For now they live in https://github.com/justinchuby/lintrunner-adapters

@justinchuby
Copy link
Collaborator Author

Alternatively we can create a new repo in the ms org and share it as a submodule across the various onnx projects

@titaiwangms
Copy link
Contributor

Alternatively we can create a new repo in the ms org and share it as a submodule across the various onnx projects

Feel like it makes more sense that each repo has its own adapters though. Or it could affect multiple repos, if one of the repo wanted a new formatting.

@justinchuby
Copy link
Collaborator Author

justinchuby commented Nov 11, 2022

I agree. I feel we can allow max control by having the adapters local to each repo. If we see a more reusable pattern we can then move them to the same place.

I thought about it again and realized things are much easier and cleaner with a package. I created https://pypi.org/project/lintrunner-adapters/.

Format and config

Add license

Add black

black

Black isort

Lintrunner config

Specify versions

Lint

Fixme

Run lintrunner

Update readme

Fix dependency

Fix config

Update dlint

Use sarif

update pyproject

Try this

Revert "Try this"

This reverts commit 95ac6dc.

Fix docstyle

pydocstyle reference

Update sarif converter

Fill lines

# exclude from sdist
exclude *.onnx
# include
Copy link
Collaborator Author

@justinchuby justinchuby Nov 13, 2022

Choose a reason for hiding this comment

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

Normalized line endings to POSIX

@justinchuby
Copy link
Collaborator Author

@gramalingam PTAL. I think this PR is now purely formatting. Should be straight forward (hopefully)

@justinchuby justinchuby merged commit d47bfd1 into main Nov 14, 2022
@justinchuby justinchuby deleted the justinchu/lintrunner branch November 14, 2022 18:59
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