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

Enable Pylint in CI and fix its errors #311

Merged
merged 2 commits into from
Nov 5, 2024
Merged

Conversation

Timmmm
Copy link
Contributor

@Timmmm Timmmm commented Nov 4, 2024

The main fixes were:

  • Specify encoding for all file opens. By default it depends on environment variables which is bad.
  • Use with to open files. Otherwise they don't necessarily get closed.

There were also a few minor things like using enumerate, not using objects as default arguments, etc. In some cases I slightly refactored the code.

Note, this PR includes #310 because one of Pylint's checks is to not use wildcard imports. I suggest we merge that one first, then I'll rebase this.

@aswaterman
Copy link
Member

CI is failing, presumably because we need to pick a different version of pylint:

An unexpected error has occurred: CalledProcessError: command: ('/home/runner/.cache/pre-commit/repo7k6kzu03/py_env-python3/bin/python', '-mpip', 'install', '.')
return code: 1
stdout:
    Processing /home/runner/.cache/pre-commit/repo7k6kzu03
      Installing build dependencies: started
      Installing build dependencies: finished with status 'done'
      Getting requirements to build wheel: started
      Getting requirements to build wheel: finished with status 'done'
      Preparing metadata (pyproject.toml): started
      Preparing metadata (pyproject.toml): finished with status 'done'
    Collecting platformdirs>=2.2.0 (from pylint==3.3.1)
      Using cached platformdirs-4.3.6-py3-none-any.whl.metadata (11 kB)
    INFO: pip is looking at multiple versions of pylint to determine which version is compatible with other requirements. This could take a while.
stderr:
    ERROR: Ignored the following yanked versions: 2.13.0, 2.13.1
    ERROR: Ignored the following versions that require a different python version: 3.3.0 Requires-Python >=3.9.0; 3.3.1 Requires-Python >=3.9.0; 3.3.2 Requires-Python >=3.9.0; 3.3.3 Requires-Python >=3.9.0; 3.3.4 Requires-Python >=3.9.0; 3.3.5 Requires-Python >=3.9.0
    ERROR: Could not find a version that satisfies the requirement astroid<=3.4.0-dev0,>=3.3.4 (from pylint) (from versions: 1.0.0, 1.0.1, 1.1.0, 1.1.1, 1.2.0, 1.2.1, 1.3.0, 1.3.1, 1.3.2, 1.3.3, 1.3.4, 1.3.5, 1.3.6, 1.3.7, 1.3.8, 1.4.0, 1.4.1, 1.4.2, 1.4.3, 1.4.4, 1.4.5, 1.4.6, 1.4.7, 1.4.8, 1.4.9, 1.5.0, 1.5.1, 1.5.2, 1.5.3, 1.6.0, 1.6.1, 1.6.2, 1.6.3, 1.6.4, 1.6.5, 1.6.6, 2.0.0.dev0, 2.0.0.dev1, 2.0.0.dev2, 2.0.0.dev3, 2.0.0.dev4, 2.0, 2.0.1, 2.0.2, 2.0.3, 2.0.4, 2.1.0.dev0, 2.1.0, 2.2.0.dev0, 2.2.0.dev1, 2.2.0, 2.2.1, 2.2.2, 2.2.3, 2.2.4, 2.2.5, 2.3.0, 2.3.1, 2.3.2, 2.3.3, 2.4.0, 2.4.1, 2.4.2, 2.5, 2.5.1, 2.5.2, 2.5.3, 2.5.4, 2.5.5, 2.5.6, 2.5.7, 2.5.8, 2.6.0.dev0, 2.6.0, 2.6.1, 2.6.2, 2.6.3, 2.6.4, 2.6.5, 2.6.6, 2.7.0, 2.7.1, 2.7.2, 2.7.3, 2.8.0, 2.8.1, 2.8.2.dev0, 2.8.2, 2.8.3, 2.8.4, 2.8.5, 2.8.6, 2.9.0, 2.9.1, 2.9.2, 2.9.3, 2.10.0, 2.11.0, 2.11.1, 2.11.2, 2.11.3, 2.11.4, 2.11.5, 2.11.6, 2.11.7, 2.12.0, 2.12.1, 2.12.2, 2.12.3, 2.12.4, 2.12.5, 2.12.6, 2.12.7, 2.12.8, 2.12.9, 2.12.10, 2.12.11, 2.12.12, 2.12.13, 2.12.14, 2.13.2, 2.13.3, 2.13.4, 2.13.5, 2.14.0, 2.14.1, 2.14.2, 2.15.0, 2.15.1, 2.15.2, 2.15.3, 2.15.4, 2.15.5, 2.15.6, 2.15.7, 2.15.8, 3.0.0a1, 3.0.0a2, 3.0.0a3, 3.0.0a4, 3.0.0a5, 3.0.0a6, 3.0.0a7, 3.0.0a8, 3.0.0a9, 3.0.0b0, 3.0.0, 3.0.1, 3.0.2, 3.0.3, 3.1.0, 3.2.0, 3.2.1, 3.2.2, 3.2.3, 3.2.4)
    ERROR: No matching distribution found for astroid<=3.4.0-dev0,>=3.3.4
Check the log at /home/runner/.cache/pre-commit/pre-commit.log

@IIITM-Jay
Copy link
Member

CI is failing, presumably because we need to pick a different version of pylint:

@aswaterman yes, you are absolutely correct here. Actually, as pylint v3.3.1 requires Python 3.9 or later, which might be the cause I think. but as per the Python related versions, we already discussed in the #299 , regarding support of later versions and multiple versions as well.

@aswaterman
Copy link
Member

Now that I have merged #299, can you suggest to @Timmmm how best to proceed?

@Timmmm
Copy link
Contributor Author

Timmmm commented Nov 5, 2024

Ah that's unfortunate. I think the simplest and best solution is just to skip Pylint on everything except the latest Python. There's no reason to run it on older versions because it doesn't use the current Python version to dictate what checks to run.

We can do the same for Pyright.

.pylintrc Outdated
@@ -0,0 +1,31 @@
[MAIN]
py-version = 3.6.0
Copy link
Member

Choose a reason for hiding this comment

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

@Timmmm I think we could use py-version as 3.9.0.

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 think we want 3.6.0. According to this it just hides suggestions for features that are only supported in newer versions of Python, so it should be set to the oldest version we support. In other words if we set it to 3.9.0 it will tell us to do some things that are not supported in 3.6.0, so we can't do them, like using the walrus operator for example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah wait I didn't see the discussion about increasing the minimum version to 3.9 ( 🎉 ). In that case I agree. I'll update the Pyright version too.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, as per the tagged PRs earlier, we would not be using the older versions of Python that are deprecated and reached its EOL. Hence, we are only supporting this repository to the latest and the supported versions of Python.
And in #299 , we found that version python 3.6 is deprecated and reached its EOL, due to which we agreed not to support versions that are deprectaed by Python community itself.

Copy link
Member

Choose a reason for hiding this comment

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

@Timmmm I think by force pushing by your end deleted my commit to use 3.9 version and as a consequence my authorship to that commit.

Copy link
Member

Choose a reason for hiding this comment

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

BTW It's OK

Copy link

codecov bot commented Nov 5, 2024

Codecov Report

Attention: Patch coverage is 98.01980% with 2 lines in your changes missing coverage. Please review.

Project coverage is 96.53%. Comparing base (25c09e6) to head (8eeb4a5).
Report is 14 commits behind head on master.

Files with missing lines Patch % Lines
shared_utils.py 90.90% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #311      +/-   ##
==========================================
+ Coverage   95.87%   96.53%   +0.65%     
==========================================
  Files          10       10              
  Lines         752      750       -2     
==========================================
+ Hits          721      724       +3     
+ Misses         31       26       -5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@IIITM-Jay IIITM-Jay requested a review from rpsene November 5, 2024 13:51
Use explicit imports rather than wildcards. This is more maintainable.
The main fixes were:

* Specify encoding for all file opens. By default it depends on environment variables which is bad.
* Use `with` to open files. Otherwise they don't necessarily get closed.

There were also a few minor things like using `enumerate`, not using objects as default arguments, etc. In some cases I slightly refactored the code.
@IIITM-Jay
Copy link
Member

@aswaterman I have configured the pylint to use version 3.9 as the pylint version v3.3.1 requires Python 3.9 as I suggested @Timmmm too in my review comments.

I think the CI passes now.

Copy link
Member

@aswaterman aswaterman left a comment

Choose a reason for hiding this comment

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

Let’s run with it.

@aswaterman aswaterman merged commit 359a943 into riscv:master Nov 5, 2024
12 checks passed
@IIITM-Jay IIITM-Jay mentioned this pull request Nov 5, 2024
@Timmmm Timmmm deleted the pylint branch November 5, 2024 16:08
Myrausman pushed a commit to Myrausman/riscv-opcodes that referenced this pull request Nov 14, 2024
* Remove wildcard imports

Use explicit imports rather than wildcards. This is more maintainable.

* Enable Pylint in CI and fix its errors

The main fixes were:

* Specify encoding for all file opens. By default it depends on environment variables which is bad.
* Use `with` to open files. Otherwise they don't necessarily get closed.

There were also a few minor things like using `enumerate`, not using objects as default arguments, etc. In some cases I slightly refactored the code.
Myrausman pushed a commit to Myrausman/riscv-opcodes that referenced this pull request Nov 14, 2024
* Remove wildcard imports

Use explicit imports rather than wildcards. This is more maintainable.

* Enable Pylint in CI and fix its errors

The main fixes were:

* Specify encoding for all file opens. By default it depends on environment variables which is bad.
* Use `with` to open files. Otherwise they don't necessarily get closed.

There were also a few minor things like using `enumerate`, not using objects as default arguments, etc. In some cases I slightly refactored the code.
Myrausman pushed a commit to Myrausman/riscv-opcodes that referenced this pull request Nov 14, 2024
* Remove wildcard imports

Use explicit imports rather than wildcards. This is more maintainable.

* Enable Pylint in CI and fix its errors

The main fixes were:

* Specify encoding for all file opens. By default it depends on environment variables which is bad.
* Use `with` to open files. Otherwise they don't necessarily get closed.

There were also a few minor things like using `enumerate`, not using objects as default arguments, etc. In some cases I slightly refactored the code.
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.

3 participants