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 pre commit #66

Merged
merged 14 commits into from
Oct 30, 2023
Merged

Add pre commit #66

merged 14 commits into from
Oct 30, 2023

Conversation

juraskov
Copy link
Member

I added pre-commit hooks to check the files before committing. Additionally, I added checking the files by Ruff linter to catch possible bugs. This PR also includes changes suggested by Ruff, fixed either automatically or manually. At this stage, Ruff does not check for formatting issues, this will be added in follow-up PR. Coded with a help of @danielhollas

@juraskov juraskov requested a review from Hanwen1018 October 27, 2023 11:53
Copy link
Contributor

@danielhollas danielhollas left a comment

Choose a reason for hiding this comment

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

Looks great. Only needs a new section in README explaining pre-commit usage.

@@ -0,0 +1,21 @@
default_language_version:
# all hooks should run with python 3.6+
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I think the current version of pre-commit requires at least python 3.8

README.md Outdated

`Pre-commit` will then run automatically at each commit and will take care of installation and running of `Ruff`.

## References
Copy link
Member

Choose a reason for hiding this comment

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

not sure I agree with this change – this section provides the citation people should use, rather than a list of references

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, I have seen both, but I agree that Citations is more precise, so I changed it. I will add there also other papers developing mlp-train once published.

Copy link
Member

Choose a reason for hiding this comment

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

❤️

@@ -13,7 +14,7 @@ class MLPotential(ABC):

def __init__(self,
name: str,
system: 'mlptrain.System'):
system: 'mlt.System'):
Copy link
Member

Choose a reason for hiding this comment

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

it would be really nice to make the type checking correct with something like this – appreciate that's a bunch of work though. feel free to ignore 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

Heh, yes, this is on my TODO list, right now I did only a quick fix to make Ruff happy. Thanks for providing the example and for review! :)

@juraskov juraskov merged commit 41fbb2c into duartegroup:main Oct 30, 2023
3 checks passed
@juraskov juraskov deleted the add-pre-commit branch October 30, 2023 10:27
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