Skip to content

Add pre-commit to dependencies with appropriate config#892

Merged
emanuel-schmid merged 11 commits intodevelopfrom
pre-commit-configuration
Oct 20, 2024
Merged

Add pre-commit to dependencies with appropriate config#892
emanuel-schmid merged 11 commits intodevelopfrom
pre-commit-configuration

Conversation

@peanutfun
Copy link
Copy Markdown
Member

@peanutfun peanutfun commented Jun 13, 2024 •
edited
Loading

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
Copy Markdown
Member Author

peanutfun commented Jun 19, 2024

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

@spjuhel
Copy link
Copy Markdown
Collaborator

spjuhel commented Jun 19, 2024

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

@peanutfun
Copy link
Copy Markdown
Member Author

peanutfun commented Jul 11, 2024

@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
manniepmkam reviewed Jul 17, 2024
View reviewed changes
@manniepmkam
Copy link
Copy Markdown
Collaborator

manniepmkam commented Jul 17, 2024

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!"

luseverin
luseverin reviewed Jul 18, 2024
View reviewed changes
Copy link
Copy Markdown
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!

@ValentinGebhart ValentinGebhart self-requested a review July 18, 2024 12:47
ValentinGebhart
ValentinGebhart reviewed Jul 18, 2024
View reviewed changes
ValentinGebhart
ValentinGebhart reviewed Jul 18, 2024
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Member Author

peanutfun commented Jul 18, 2024

@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 commented Jul 19, 2024
View reviewed changes
@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
@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

Reviewers

@manniepmkam manniepmkam manniepmkam left review comments

@emanuel-schmid emanuel-schmid emanuel-schmid left review comments

@ValentinGebhart ValentinGebhart ValentinGebhart left review comments

@luseverin luseverin luseverin left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@peanutfun @spjuhel @manniepmkam @emanuel-schmid @ValentinGebhart @luseverin