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 more ruff rules including A, DTZ, PGH and more #3871

Merged
merged 50 commits into from
Jun 21, 2024

Conversation

DanielYang59
Copy link
Contributor

@DanielYang59 DanielYang59 commented Jun 9, 2024

Summary

Enable more ruff rules

The following requires code change:

Caution

b1aa5bb and 50e9b7d changed multiple ValueError to more semantic TypeError to fix TRY004. this is breaking. however, downstream repercussions are expected to be small.

pinging @mkhorton @shyuep for approval

Future PR

Other format tweaks

  • Replace match.group(x) with match[x]
  • Replace array[0:x] slicing with array[:x]
  • Replace already multi-lined dict() constructor with {} for speed (almost 2x-4x speedup).

@DanielYang59 DanielYang59 marked this pull request as ready for review June 9, 2024 06:43
@JaGeo
Copy link
Member

JaGeo commented Jun 9, 2024

@DanielYang59 could you trigger the atomate2 tests?

@DanielYang59
Copy link
Contributor Author

DanielYang59 commented Jun 9, 2024

@JaGeo Thanks for commenting. Can you let me how to achieve that? It should only be possible when pushing to master I guess?

if: github.ref == 'refs/heads/master'

This PR is still a type annotation improvement, and I don't expect much code functionality change :)

@JaGeo
Copy link
Member

JaGeo commented Jun 9, 2024

@DanielYang59 ah okay. Then, we will see after merging! Thanks!

@DanielYang59 DanielYang59 marked this pull request as draft June 9, 2024 07:29
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
tests/core/test_structure.py Outdated Show resolved Hide resolved
@janosh janosh added linting Linting and quality assurance breaking Breaking change types Type all the things labels Jun 9, 2024
@DanielYang59
Copy link
Contributor Author

DanielYang59 commented Jun 10, 2024

@janosh I replaced some already multi-lined dictionary constructor dict() with {} for speed in 47d72f3, I know you don't want to introduce extra line, so I just replaced those that wouldn't do so.

From my side, the speed up is something I really want to have (2x - 4x, though being in the nanosecond level) :)

@DanielYang59
Copy link
Contributor Author

DanielYang59 commented Jun 21, 2024

@shyuep Can I have your approval on the following potentially breaking change/fix (change some ValueError to TypeError where the type doesn't match)? Thanks!

b1aa5bb and 50e9b7d changed multiple ValueError to more semantic TypeError to fix TRY004. this is breaking. however, downstream repercussions are expected to be small.

@shyuep
Copy link
Member

shyuep commented Jun 21, 2024

If the errors are not the type that are meant to be caught, it is fine to change the error type. What I mean is that if the error is something that happens because of a bad use of code. Rather than something like catching an expected exception case and then handling it in some way.

@DanielYang59
Copy link
Contributor Author

DanielYang59 commented Jun 21, 2024

Thanks for your reply!

Yes I could confirm those are exceptions we raise instead of catch (try...except...), see b1aa5bb and 50e9b7d.

Mostly in the following pattern:

if not isinstance(variable, expected_type):
-    raise ValueError("variable should be in {expected_type}")
+    raise TypeError("variable should be in {expected_type}")

Still it could be breaking if any downstream operation replies on catching a ValueError, but I would consider this a fix as the wrong type of exception was raised in the first place.

@shyuep shyuep merged commit cb177f4 into materialsproject:master Jun 21, 2024
33 checks passed
@shyuep
Copy link
Member

shyuep commented Jun 21, 2024

Thanks I have merged them,.

@DanielYang59 DanielYang59 deleted the pgh-ruff-rules branch June 22, 2024 00:39
@DanielYang59
Copy link
Contributor Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Breaking change linting Linting and quality assurance types Type all the things
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Dev] datetime.datetime.utcnow() deprecated and replacement breaks strptime
4 participants