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

Move pull request guidelines to SIMULATeQCD/community #39

Open
clarkedavida opened this issue Jan 2, 2022 · 4 comments
Open

Move pull request guidelines to SIMULATeQCD/community #39

clarkedavida opened this issue Jan 2, 2022 · 4 comments
Assignees
Labels
documentation Improvements or additions to documentation high priority This needs urgent attention.

Comments

@clarkedavida
Copy link
Collaborator

We have the "How to do pull requests" documentation; I think it makes sense to include our guidelines there. I have already included some steps to take when merging a feature into the main branch. I think we should also include some extra guidelines for a pull request, to help us check that the merger did everything correctly. For example:

  • I noticed that people are again writing tests are not really tests; i.e. they don't have a clear pass/fail condition. I am trying to include all the tests in scripts/TEST_run.bash, and without it returning some kind of error, the script just assumes it passed. This leads to some "tests" that are never really being checked. We should not allow someone to merge until they've implemented this.
  • Along these lines, I am seeing people put executables in the wrong place, like profilers in the testing folder.
    We can probably think of more stuff. Maybe we can make the guideline that when someone submits a pull request, they have to explicitly tell us a few things, like
  • I confirm that I did make -j everything and it all compiles.
  • I confirm that I ran TEST_run.bash and everything passed.
  • Here are the new tests I wrote, please look at them
    Anything you guys would like to add?
@clarkedavida clarkedavida added the documentation Improvements or additions to documentation label Jan 2, 2022
@clarkedavida clarkedavida self-assigned this Jan 2, 2022
@luhuhis
Copy link
Collaborator

luhuhis commented Jan 7, 2022

People mainly run tests when they have changed the code (added features, etc.). Especially for new users, it would be nice to have everything you need to know about "Contributing to the code", including everything about the test suite, in one place, and not scattered across multiple pages in the documentation. It should be as easy as possible for new users to add code. Maybe we should put a section about this into the readme?

@clarkedavida
Copy link
Collaborator Author

I agree with this. I tend to agree that our documentation is split up a little bit too much.

I also really like what you did with these TL;DR admonitions. Can you set up something like that for contributing to the code? I personally find it really helpful to have a quick overview of what I need to do, and then I can look into each step in detail when I need to.

@lukas-mazur
Copy link
Collaborator

Yes, I agree

@luhuhis
Copy link
Collaborator

luhuhis commented Jan 7, 2022

Maybe there is an even better place for this, which you can find here:
https://github.com/LatticeQCD/SIMULATeQCD/community
There are multiple things that are still missing there, which we should add.

@luhuhis luhuhis added the high priority This needs urgent attention. label Jan 7, 2022
@lukas-mazur lukas-mazur changed the title Pull request guidelines Move pull request guidelines to SIMULATeQCD/community May 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation high priority This needs urgent attention.
Projects
None yet
Development

No branches or pull requests

3 participants