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

Further improves CONTRIBUTING #334

Merged
merged 7 commits into from
Mar 1, 2022
Merged

Further improves CONTRIBUTING #334

merged 7 commits into from
Mar 1, 2022

Conversation

joaomcteixeira
Copy link
Member

Completes the CONTRIBUTING guidelines. This should be enough. The CONTRIBUTING file is not an explanation of how to use github or how to dev code, but how to interact with the project. Do you find something missing? You can read the rendered markdown version of the file here.

CONTRIBUTING.md Outdated Show resolved Hide resolved
Copy link
Member

@rvhonorato rvhonorato left a comment

Choose a reason for hiding this comment

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

Looks good overall, would also be helpful if there was a checklist (maybe as a template). Based on what you wrote, would be something like


Conditions for PR to be approved:

  • PR contains only Python code
  • PR does not add dependency
  • PR does not contain Python Classes
  • PR follows code style
  • PR passes tests
  • PR does not break license

If any of these criteria are not met, add a detailed justification below:
...


@joaomcteixeira
Copy link
Member Author

joaomcteixeira commented Feb 22, 2022

Thanks Rodrigo. I will add those in the PR template. Okay Alexandre, will add that info too.

@codecov-commenter
Copy link

codecov-commenter commented Feb 22, 2022

Codecov Report

Merging #334 (7e42d57) into main (ce56df2) will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #334      +/-   ##
==========================================
+ Coverage   57.73%   57.74%   +0.01%     
==========================================
  Files          61       61              
  Lines        3712     3711       -1     
==========================================
  Hits         2143     2143              
+ Misses       1569     1568       -1     
Impacted Files Coverage Δ
src/haddock/modules/refinement/emref/__init__.py 24.19% <0.00%> (ø)
src/haddock/modules/refinement/mdref/__init__.py 24.59% <0.00%> (ø)
src/haddock/modules/refinement/flexref/__init__.py 24.19% <0.00%> (ø)
src/haddock/modules/sampling/rigidbody/__init__.py 24.59% <0.00%> (ø)
src/haddock/libs/libcns.py 36.62% <0.00%> (+0.21%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ce56df2...7e42d57. Read the comment docs.

Copy link
Member

@rvhonorato rvhonorato left a comment

Choose a reason for hiding this comment

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

Changed the grammar of some bits, see if you agree.

-test.cfg examples

This is real-world Doublethink 🐑

@joaomcteixeira
Copy link
Member Author

Great 👍
Sorry, I normally format .md files to max 72 lines. But that's no reason for that, or 80, or whatever, as long as they are not 400 chars lines. It's perfectly fine as you did. If @amjjbonvin agrees we can merge 😉

Copy link
Member

@rvhonorato rvhonorato left a comment

Choose a reason for hiding this comment

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

Actually I just thought of something else; I am fine with following arbitrary rules (as long as they are clear, and I think this PR makes it much more clear), but since we want to have a community developing for haddock3, would be nice if the rules had some sort of motivation - if you think its relevant, of course.

For example:

  • Only python code - why?
  • Avoid use of classes - many of us when we learn python, classes are there on lesson 2. So why not use them? How do they affect HADDOCK in a bad way?
  • Does not add dependencies - if we are working on an open source and community driven project, why not use things that the community have developed? doesn't this go against Don't Repeat Yourself paradigm?
  • Why attach testing to tox? (This makes tox a dependency)
  • Shouldn't different modules be allowed different styles? They should be indeed modular and separate from the main code.
  • What is considered "well documented", docstrings with parameters and return or just a lot of # on top of each line?

Just a few points that I can think off, of course if you don't agree with answering this or don't think this is needed for the community, also fine.

@amjjbonvin
Copy link
Member

amjjbonvin commented Feb 23, 2022 via email

@rvhonorato
Copy link
Member

Based on what we discussed elsewhere, I've added a new step 6 that explains the motivations behind the contribution rules so that it is also clear for someone outside the group, think this one can be merged! Thanks for making this clear @joaomcteixeira ;)

@joaomcteixeira
Copy link
Member Author

Made some improvements for clarity

@joaomcteixeira joaomcteixeira merged commit 0707193 into main Mar 1, 2022
@joaomcteixeira joaomcteixeira deleted the contri branch March 1, 2022 09:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improve docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants