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

Same ruff linting as jf-remote #42

Merged
merged 9 commits into from
Jan 20, 2025

Conversation

janosh
Copy link
Collaborator

@janosh janosh commented Jul 31, 2024

equivalent qtoolkit PR to Matgenix/jobflow-remote#134

@janosh
Copy link
Collaborator Author

janosh commented Jul 31, 2024

@ml-evs would be great if you could have a look at these changes! 🙏

@ml-evs
Copy link
Member

ml-evs commented Aug 2, 2024

Thanks @janosh, this PR fine in principle, but would prefer to hold off until #43 is worked on a bit to avoid unnecessary churn, and probably to wait until @gpetretto is back from vacation for a final check.

Copy link
Contributor

@gpetretto gpetretto left a comment

Choose a reason for hiding this comment

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

I think there may have been a small change in the logic in one point (see comment). Aside from this, I am fine with the PR. As @ml-evs suggested I would wait for #43 to be merged before proceeding with this.

src/qtoolkit/manager.py Show resolved Hide resolved
@davidwaroquiers
Copy link
Member

@janosh hope you are fine.
I think this can be picked up again on your side as #43 has been merged now.
Thanks so much for your contribution!

@gpetretto gpetretto force-pushed the same-ruff-linting-as-jf-remote branch from 2a3ba1c to cf6f479 Compare January 20, 2025 14:17
@gpetretto
Copy link
Contributor

Hi @janosh, just wanted to notify you that, since we wanted to finalize this before making a new release of qtoolkit, I updated your PR to the latest commits and fixed the remaining linting issues.

Copy link
Member

@davidwaroquiers davidwaroquiers left a comment

Choose a reason for hiding this comment

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

Seems all good to me. I have 3 small comments but they are more like questions, maybe no need to change/do anything.

doc/source/conf.py Outdated Show resolved Hide resolved
Comment on lines +8 to +10
authors = [
{ name = "David Waroquiers", email = "david.waroquiers@matgenix.com" },
]
Copy link
Member

Choose a reason for hiding this comment

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

Not really related to this PR but I'd add other authors here no ? Who ?

pyproject.toml Show resolved Hide resolved
@gpetretto gpetretto merged commit 415eacd into Matgenix:develop Jan 20, 2025
5 checks passed
@gpetretto
Copy link
Contributor

Thanks all for the contribution

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