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

Lighter switch to ruff #2998

Merged
merged 31 commits into from
Jan 22, 2024
Merged

Lighter switch to ruff #2998

merged 31 commits into from
Jan 22, 2024

Conversation

jhale
Copy link
Member

@jhale jhale commented Jan 18, 2024

Ruff linting without consistent formatting (e.g. black).

@jhale jhale mentioned this pull request Jan 18, 2024
@jhale
Copy link
Member Author

jhale commented Jan 18, 2024

We cannot use ruff's isort today, it defaults to black-compatible import formatting.

@@ -213,7 +213,7 @@ def _assemble_vector_array(b: np.ndarray, L: Form, constants=None, coeffs=None):


@functools.singledispatch
def assemble_matrix(a: typing.Any, bcs: typing.Optional[typing.List[DirichletBC]] = None,
def assemble_matrix(a: typing.Any, bcs: typing.Optional[list[DirichletBC]] = None,
Copy link
Member

Choose a reason for hiding this comment

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

This change, and all similar ones, will break python < 3.9. I think that ruff uses by default whatever version of python is available in the docker image (3.10, if I remember correctly), but there is a configuration option to prevent it from doing that. However, I don't remember which of the rules is enforcing this change (pyupgrade maybe?) to check the actual name of this option.

Copy link
Member Author

Choose a reason for hiding this comment

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

We should discuss deprecating 3.8, only has ~6 months of support left.

Copy link
Member Author

@jhale jhale Jan 18, 2024

Choose a reason for hiding this comment

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

I checked this. The target version is inferred from pyproject.toml.

ruff --show-settings | grep target_version

gives

target_version: Py38,
target_version: Py38,

@@ -18,8 +18,8 @@
import basix.ufl
import ufl
from dolfinx import cpp as _cpp
from dolfinx.cpp.io import perm_gmsh as cell_perm_gmsh # noqa F401
from dolfinx.cpp.io import perm_vtk as cell_perm_vtk # noqa F401
from dolfinx.cpp.io import perm_gmsh as cell_perm_gmsh # F401
Copy link
Member

Choose a reason for hiding this comment

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

Removing the noqa is odd: I guess it should have been either noqa: F401, or no comment at all

python/pyproject.toml Show resolved Hide resolved
@@ -17,7 +17,7 @@
from dolfinx import default_real_type
from dolfinx.fem import Function, functionspace
from dolfinx.io import VTKFile
from dolfinx.io.utils import cell_perm_vtk # noqa F401
Copy link
Member

Choose a reason for hiding this comment

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

noqa here too.

@@ -194,7 +194,8 @@
u.x.array[:] = 0.0

# Interpolate initial condition
u.sub(0).interpolate(lambda x: 0.63 + 0.02 * (0.5 - np.random.rand(x.shape[1])))
rng = np.random.default_rng()

Check notice

Code scanning / SonarCloud

Results that depend on random number generation should be reproducible Low

Provide a seed for this random generator. See more on SonarCloud
Copy link
Member

Choose a reason for hiding this comment

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

I guess there is value in this comment, but how come it did not notice it before? The call to np.random.rand was already there.

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

I think it is a new feature of Github, Lets add a seed?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think GitHub can pickup on new issues picked up by SonarCloud and now put them back in to the review, which seems very useful.

@jhale jhale enabled auto-merge January 22, 2024 17:30
@jhale jhale added this pull request to the merge queue Jan 22, 2024
Merged via the queue into main with commit f85b780 Jan 22, 2024
19 checks passed
@jhale jhale deleted the jhale/ruff-light branch January 22, 2024 18:32
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