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

Add Ruff to pre-commit #819

Merged
merged 23 commits into from
May 6, 2024
Merged

Add Ruff to pre-commit #819

merged 23 commits into from
May 6, 2024

Conversation

CalCraven
Copy link
Contributor

This is just a test PR to test the linting with Ruff, which is recommended by pydocstyle as a replacement and used by other big python package.

ryckaert_connection_type = convert_opls_to_ryckaert(
opls_connection_type
)
ryckaert_connection_type = convert_opls_to_ryckaert(opls_connection_type)

Check notice

Code scanning / CodeQL

Unused local variable Note

Variable ryckaert_connection_type is not used.
gmso/tests/test_potential.py Fixed Show fixed Hide fixed
gmso/tests/test_convert_mbuild.py Fixed Show fixed Hide fixed
ryckaert_connection_type = convert_opls_to_ryckaert(
opls_connection_type
)
ryckaert_connection_type = convert_opls_to_ryckaert(opls_connection_type)

Check warning

Code scanning / CodeQL

Variable defined multiple times Warning

This assignment to 'ryckaert_connection_type' is unnecessary as it is
redefined
before this value is used.
gmso/tests/test_topology.py Fixed Show fixed Hide fixed
connection_lines = open(filename, "r").readlines()[
i + 2 : i + n_connections + 2
]
connection_lines = open(filename, "r").readlines()[i + 2 : i + n_connections + 2]

Check warning

Code scanning / CodeQL

File is not always closed Warning

File is opened but is not closed.
@CalCraven
Copy link
Contributor Author

Looks like the pre-commit has a lot of recommendations for changes. @daico007 I think we should look at some custom parsing, because notes like the __init__.py shouldn't be following the same parsing guidelines.

@chrisjonesBSU
Copy link
Contributor

If the only issue is the F401 errors in the __init__.py files then maybe we can just ignore that specific error in those files.

See this in the documentation

@CalCraven
Copy link
Contributor Author

If the only issue is the F401 errors in the init.py files then maybe we can just ignore that specific error in those files.

Yes that would be good. We may have to do that with a few of the different files this way because it's a bunch of files across the project.

@chrisjonesBSU
Copy link
Contributor

Reading the ruff documentation https://docs.astral.sh/ruff/linter/#error-suppression

We can add # ruff: noqa: F401 to the top of any __init__.py files which may be easier than setting the paths to ignore in a config file.

@chrisjonesBSU
Copy link
Contributor

The rest of the suggested changes seem pretty reasonable I think. What do you all think @CalCraven @daico007 ?

@CalCraven
Copy link
Contributor Author

I would say we should ignore things for the tests module. Otherwise, I think it would be wise to perform the rest of the changes just to improper our PEP compliance.

gmso/tests/test_convert_mbuild.py Dismissed Show dismissed Hide dismissed
@chrisjonesBSU
Copy link
Contributor

chrisjonesBSU commented May 2, 2024

I think I got most of the errors fixed (even in the tests). There are some lingering ones about not using lambda and not using bare excepts.

I think we should fix the bare excepts, I just don't know what the errors should be off the top of my head. We use lambda functions quite a bit, so maybe we just ignore that specific error in the config file?

forcefield_selection_with_paths[residue] = (
ff_names_path_iteration
)
read_xlm_iteration = minidom.parse(ff_names_path_iteration) # noqa: F841

Check warning

Code scanning / CodeQL

Variable defined multiple times Warning

This assignment to 'read_xlm_iteration' is unnecessary as it is
redefined
before this value is used.
@CalCraven
Copy link
Contributor Author

https://github.com/BrianPugh/python-template/blob/main/.pre-commit-config.yaml

Going to link to a cookie-cutter version of a ruff linter, check out the pyproject.toml as well for some additional formatting stuff we could consider implementing, such as type checking and isort.

Copy link
Contributor

@chrisjonesBSU chrisjonesBSU left a comment

Choose a reason for hiding this comment

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

LGTM, just left one comment about how we want to handle automatic fixes on PRs.

hooks:
# Run the linter.
- id: ruff
args: [--line-length=80]
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add the fix flag back in, so that easy formatting fixes are automatically made and comitted on PRs?

Copy link
Member

Choose a reason for hiding this comment

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

yeah, sure! we can always turn it off later if it's become troublesome.

@daico007 daico007 merged commit 069170b into mosdef-hub:main May 6, 2024
12 checks passed
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