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

use ruff as linter and fix lots of problems #450

Merged
merged 17 commits into from Feb 9, 2023
Merged

Conversation

FHU-yezi
Copy link
Contributor

@FHU-yezi FHU-yezi commented Feb 5, 2023

All Submissions:

  • Have you followed the guidelines stated in CONTRIBUTING.md file?
  • Have you checked to ensure there aren't any other open Pull Requests for the desired changed?

Type of change

Please delete options that are not relevant.

  • This change need a CI pipeline update. ( I have already changed the build workflow )
  • This change is about a development tools change.

New Feature Submission:

  • Does your submission pass the tests?
  • Have you linted your code locally prior to submission?

Changes To Core Features:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your core changes, as applicable?
  • Have you successfully ran tests with your changes locally?

After these steps, you're ready to open a pull request.

a. Give a descriptive title to your PR.

b. Describe your changes.

c. Put `closes #XXXX` in your comment to auto-close the issue that your PR fixes (if such).

Ruff is a super-fast Python code linter written in Rust. This Pull Request removed the old pydocstyle linter and isort import reorder tool, the ruff documentation said it can be a replacement of these tools.

I also changed .github/workflows/build.yml file to let the CI pipeline running correctly ( maybe? ), and CONTRIBUTING.md to replace pydocstyle lines with ruff check things.

With the help from ruff, I changed lots of buggy or unstable code in our code base, included some unused import in non-init files, some meaningless f-string and some missing chars in our docstring.

For editor integration, ruff has a official VS Code Extension and an unofficial PyCharm extension, Vim guys can also use official LSP called ruff-lsp.

For a performance reference, I use hyperfine to benchmark ruff ( with all the features we need turned on ) and flake8 ( with zero plugin ):

❯ hyperfine "ruff ." "flake8 ." -i
Benchmark 1: ruff .
  Time (mean ± σ):      10.6 ms ±   5.1 ms    [User: 8.4 ms, System: 5.9 ms]
  Range (min … max):     7.7 ms …  68.3 ms    202 runs
 
  Warning: Statistical outliers were detected. Consider re-running this benchmark on a quiet PC without any interferences from other programs. It might help to use the '--warmup' or '--prepare' options.
 
Benchmark 2: flake8 .
  Time (mean ± σ):      1.253 s ±  0.011 s    [User: 3.515 s, System: 0.110 s]
  Range (min … max):    1.241 s …  1.279 s    10 runs
 
  Warning: Ignoring non-zero exit code.
 
Summary
  'ruff .' ran
  118.42 ± 57.40 times faster than 'flake8 .'

Speed up development and CI, make more features with less bugs, let's "rust" it up~

@Alek99 Alek99 self-requested a review February 5, 2023 21:10
@Alek99
Copy link
Contributor

Alek99 commented Feb 6, 2023

I like this addition. At first glance it looks good I'm going to pull this branch and test it locally tomorrow and then ill approve

@Alek99
Copy link
Contributor

Alek99 commented Feb 6, 2023

I think there are some merge conflicts can you resolve these and then I'll merge approve looks good

@FHU-yezi
Copy link
Contributor Author

FHU-yezi commented Feb 7, 2023

@Alek99 Have merged all the change from upstream main branch, please review again and we are about to go.

@Alek99
Copy link
Contributor

Alek99 commented Feb 8, 2023

Ok I just tried running this code and got an error,

./pages/index.js
Error: 
  x Unexpected token `{`. Expected identifier, string literal, numeric literal or [ for the computed key
    ,----
 35 | {set_state}({{
    :              ^
    `----

Caused by:
    0: failed to process input file
    1: Syntax Error

I think when ruff took out the f strings in the compiler/templates.py its causing an error. Does this work locally for you?

@FHU-yezi
Copy link
Contributor Author

FHU-yezi commented Feb 8, 2023

I just ran the unit test and it passed. Will try to revent this change.

@FHU-yezi FHU-yezi requested review from picklelo and removed request for Alek99 February 9, 2023 06:17
@FHU-yezi
Copy link
Contributor Author

FHU-yezi commented Feb 9, 2023

Fixed this error and ready to review again.

Copy link
Contributor

@picklelo picklelo left a comment

Choose a reason for hiding this comment

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

This looks great except for pynecone/compiler/templates.py - let's undo all the changes there. The apps won't run with the code as is

pynecone/compiler/templates.py Outdated Show resolved Hide resolved
@FHU-yezi
Copy link
Contributor Author

FHU-yezi commented Feb 9, 2023

@picklelo I use git blame to locate the editor of these lines, seems it is modified by advo-kat in 0056d94d two weeks ago.

I just tried to override this file with the latest version in our master branch, please review again, sorry for this problem.

Copy link
Contributor

@picklelo picklelo left a comment

Choose a reason for hiding this comment

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

Awesome thanks for fixing!

@picklelo picklelo merged commit 1529a23 into reflex-dev:main Feb 9, 2023
ACucos1 pushed a commit to ACucos1/rd-pynecone that referenced this pull request Feb 28, 2023
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.

None yet

3 participants