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 to dependencies with appropriate config #892

Merged
merged 11 commits into from
Oct 20, 2024

Conversation

peanutfun
Copy link
Member

@peanutfun peanutfun commented Jun 13, 2024

Changes proposed in this PR:

  • Add configuration for pre-commit (hooks), but do not apply them yet.
  • Add documentation on how to install and use pre-commit hooks.

This PR fixes #

This PR will remain a Draft until after the next release.

PR Author Checklist

PR Reviewer Checklist

@peanutfun peanutfun changed the title Add pre-commit to dependencies with appropriate config WIP: Add pre-commit to dependencies with appropriate config Jun 18, 2024
@peanutfun
Copy link
Member Author

@spjuhel Do I recall correctly that you wanted to help with the documentation? Can we work together on it soon?

@spjuhel
Copy link
Collaborator

spjuhel commented Jun 19, 2024

Yes! I can find some time tomorrow afternoon or Friday anytime.

@peanutfun
Copy link
Member Author

@ValentinGebhart @luseverin @DahyannAraya Would anybody of you be interested in reviewing the additions to the Installation, Git, and Climada Conventions guides? We would be glad about input from people outside the core development team.

@manniepmkam
Copy link
Collaborator

I read through the doc/guide/Guide_CLIMADA_conventions.ipynb, looks good to me.

I guess the doc/guide/Guide_Git_Development.ipynb is not ready yet? I saw quite a few lines with "Chahan will discuss this later!"

Copy link
Collaborator

@luseverin luseverin left a comment

Choose a reason for hiding this comment

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

Hi guys,
I followed the updated guide and installed the pre-commit and black plugins for VScode. Let me know if this was the expected outcome! I also highlighted a couple typo in my review, and added a couple comments for clarifications:

  • Is it required to also install isort separately?
  • Shouldn't be black already be mentionned in the installation guide alongside the suggestion to install the pre-commit plugin ?

Unfortunately I did not have a branch to reformat with black at the moment but the instruction for reformatting a branch seemed clear to me.
Otherwise, I have nothing to add. Nice work!

doc/guide/Guide_Git_Development.ipynb Outdated Show resolved Hide resolved
doc/guide/install.rst Outdated Show resolved Hide resolved
doc/guide/Guide_CLIMADA_conventions.ipynb Outdated Show resolved Hide resolved
doc/guide/Guide_CLIMADA_conventions.ipynb Show resolved Hide resolved
@ValentinGebhart ValentinGebhart self-requested a review July 18, 2024 12:47
Copy link
Collaborator

@ValentinGebhart ValentinGebhart left a comment

Choose a reason for hiding this comment

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

To me the documentation changes are pretty clear. I followed the installation of the pre-hooks and tested them, they seem to work. I did not test the instruction for "How do I update my branch if it is not up to date with the formatted Climada?" but it also seems reasonable.

@peanutfun
Copy link
Member Author

@manniepmkam Thank you for the updates!

I guess the doc/guide/Guide_Git_Development.ipynb is not ready yet? I saw quite a few lines with "Chahan will discuss this later!"

😂 I did not even realize! Not sure what that is supposed to mean, we'll leave it as is for now.

@peanutfun peanutfun marked this pull request as ready for review August 9, 2024 08:43
@peanutfun peanutfun changed the title WIP: Add pre-commit to dependencies with appropriate config Add pre-commit to dependencies with appropriate config Aug 9, 2024
@emanuel-schmid emanuel-schmid merged commit a92196d into develop Oct 20, 2024
18 checks passed
@emanuel-schmid emanuel-schmid deleted the pre-commit-configuration branch October 20, 2024 13:14
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.

6 participants