Merge pull request #70 from su2code/update_guidelines

pcarruscag · web-flow · commit fc57e41b5214 · 2022-02-05T19:28:39.000Z
Update style guidelines
diff --git a/_docs_v7/Code-Review.md b/_docs_v7/Code-Review.md
@@ -19,19 +19,19 @@ All developers and users (internal and external) are encouraged to participate i
 2. Is the change implemented in the correct way?
 - Does it interact minimally with the rest of the code?
 - Does it have the correct algorithmic complexity?
-- Is it located in the place? (file, etc.)
+- Is it located in the right place? (file, class, etc.)
 
 3. Is the code legible?
 - Is the code mostly legible on its own without documentation?
 - Are the variable names concise and accurate?
-- Is there documentation where necessary, and it is correct?
+- Is there documentation where necessary, and is it correct?
 
 4. Does the code follow established conventions?
 - Does it match the SU2 code style?
 - Do the variable names follow the same patterns as in other parts of the code?
 
 # Good code changes
-The above list is a long list of questions. A large change to the code will be much harder to review than a small change. As such, good pull requests will contain a minimal set of changes to be useful. Pull requests should "do one thing". In some cases, "doing one thing" may be a large change to the code, such as adding a new flow solver. In most cases, changes can be done in small increments that can each individually be reviewed and evaluated. Pull requests should not be of the form "Add a new config option and fix a bug in interation_structure.cpp". These should be two separate pull requests.
+The above list is a long list of questions. A large change to the code will be much harder to review than a small change. As such, good pull requests will contain a minimal set of changes to be useful. Pull requests should "do one thing". In some cases, "doing one thing" may be a large change to the code, such as adding a new flow solver. In most cases, changes can be done in small increments that can each individually be reviewed and evaluated. Pull requests should not be of the form "Add a new config option and fix a bug in interation_structure.cpp". **Such pull requests will be required to be split**.
 
 # The Code Review Process
 Github provides an easy interface for performing code reviews as part of every Pull Request. After a Pull Request is submitted to the SU2 'develop' branch, two different developers must review and approve the code changes before the request can be merged, in addition to passing the Travis CI regression test suite. Reviewers have the opportunity to comment on the changes and requests specific changes.
@@ -40,6 +40,6 @@ In response to these comments, the pull requester should make changes to the cod
 
 When a reviewer is happy with the proposed changes to the code, the reviewer should approve and can say "LGTM", standing for "looks good to me". In general, the changes to the code should be finalized before a LGTM is given, though if there are only very minor outstanding issues an LGTM can be given along with the changes. For example, if the only outstanding issue with the PR is that a word has been misspelled, a reviewer may make an inline comment about the misspelling, and in the main dialogue say "LGTM with the comment fix". 
 
-All developers are encouraged to participate in the code review process, as the code is for everyone. However, there will typically be a specific set of developers who are experts in the section of code that is being modified. Generally, an LGTM should be gotten from at least one of these developers before merging. Users can be requested using "@username", for example, "PTAL @su2luvr". This sends that user an email about the pull request. Similarly, this can be used to request the opinions of other developers. While this can feel burdensome, it is in place to maintain good, correct code. Please use good judgement -- if the change is a spelling fix in a comment, it is not necessary to solicit the opinion of the entire development team.
+**All developers are encouraged to participate in the code review process, as the code is for everyone.** However, there will typically be a specific set of developers who are experts in the section of code that is being modified. Generally, a LGTM should be gotten from at least one of these developers before merging. Users can be requested using "@username", for example, "PTAL @su2luvr". This sends that user an email about the pull request. Similarly, this can be used to request the opinions of other developers. While this can feel burdensome, it is in place to maintain good, correct code.
 
 Once the proper set of "LGTM"s has been received, the change can be merged. If the pull-requester has commit access, it is tradition to let them make the merge. If the requester does not, then the main/final reviewer can submit the merge. If the pull-request came from an internal branch, the branch should be deleted on conclusion if it is no longer useful.
diff --git a/_docs_v7/Code-Structure.md b/_docs_v7/Code-Structure.md
@@ -5,6 +5,8 @@ permalink: /docs_v7/Code-Structure/
 
 Full details on the class hierarchy and internal structure of the code can be found in the Doxygen documentation for SU2. A brief description for the major C++ classes is given on this page.
 
+**Note:** The images below can be out of sync with the current version of the code.
+
 The objective of this section is to introduce the C++ class structure of SU2 at a high level. The class descriptions below focus on the structure within SU2_CFD (the main component of SU2), but many of the classes are also used in the other modules. Maximizing the flexibility of the code was a fundamental driver for the design of the class architecture, and an overview of the collaboration diagram of all classes within SU2_CFD is shown below. 
 
 ![Class Structure General](../../docs_files/class_c_driver__coll__graph.png)
diff --git a/_docs_v7/Gitting-Started.md b/_docs_v7/Gitting-Started.md
@@ -16,6 +16,10 @@ We follow [a popular git branching strategy](http://nvie.com/posts/a-successful-
                 
 ## Contributing Code
                 
-SU2 merges new code contributions through <a href="https://help.github.com/articles/creating-a-pull-request">pull requests</a>.  As a new developer, you'll want to <a href="https://help.github.com/articles/fork-a-repo/">fork</a> SU2 to your personal account.  This creates a clone of the whole SU2 repository, branches and all, inside your github account.  Generally you'll want to start from the develop branch, but you can check with the developers if you think it would be more appropriate to work on a feature branch.
+SU2 merges new code contributions through <a href="https://help.github.com/articles/creating-a-pull-request">pull requests</a>.  As a new developer, you'll want to <a href="https://help.github.com/articles/fork-a-repo/">fork</a> SU2 to your personal account.  This creates a clone of the whole SU2 repository, branches and all, inside your github account.  Generally you'll want to **start from the develop branch**, but you can check with the developers if you think it would be more appropriate to work on a feature branch (join the SU2 slack channel or open a GitHub discussion to get in touch).
                 
-You can push all of your working changes to your forked repository.  Once you're happy with these, and want to push them to the origin repository, submit a pull request to the 'develop' branch under the SU2 repo.  Make sure to pull any new changes from the origin repository before submitting the pull request, so that the changes can be merged more simply.  The SU2 developers will review the changes, make comments, ask for some edits.  Then when everything looks good, your changes will merge into the main development branch!
+You can push all of your working changes to your forked repository. Once you're happy with these, and want to push them to the origin repository, submit a pull request to the 'develop' branch under the SU2 repo. Make sure to **pull any new changes regularly** from the origin repository before submitting the pull request, so that the changes can be merged more simply. The SU2 developers will review the changes, make comments, ask for some edits, and when everything looks good, your changes will merge into the main development branch!
+
+### General Guidelines
+- If you have a very clear plan for the work you are doing, open a **draft pull request** so that the maintainers can start reviewing early.
+- If you are working on a large feature (1k+ lines of code (loc), or 200+ loc with changes to 10+ files) ask to be added to the SU2 organization to work from the SU2 repo directly instead of your fork (this makes reviewing easier).
diff --git a/_docs_v7/Running-Regression-Tests.md b/_docs_v7/Running-Regression-Tests.md
@@ -3,17 +3,7 @@ title: Running Regression Tests
 permalink: /docs_v7/Running-Regression-Tests/
 ---
 
-The regression tests can be run on your local machine by using the Python scripts available in the SU2/TestCases/ directory and the mesh files from the su2code/TestCases repository. See the [Test Cases](/docs_v7/Test-Cases/) page for more information on working with the TestCases repo.
+The regression tests can be run on your local machine by using the Python scripts available in the SU2/TestCases/ directory and the mesh files from the su2code/TestCases and Tutorials repository. See the [Test Cases](/docs_v7/Test-Cases/) page for more information on working with the TestCases repo.
 
-When you are ready to combine your modifications into the develop branch, creating a pull request will automatically run these same regression tests on the Travis CI system. Your pull request must pass these tests in order to be merged into the develop branch. In the pull request, you will be able to see the state of the tests, and by clicking on links find the details of the test results. 
+When you are ready to combine your modifications into the develop branch, creating a pull request will automatically run these same regression tests. Your pull request must pass these tests in order to be merged into the develop branch. In the pull request, you will be able to see the state of the tests, and by clicking on links find the details of the test results. 
 
-If you are working with a forked version of the repository, you can use the following directions to run these same regression tests within your repository rather than within the su2code/ repository. This is preferable if you are not ready to submit your code to the develop branch and just want to run the tests, or if you want to create your own regression tests.
- 
-1. Modify the travis.yml file in the develop branch to update the ***email address*** and ***repository url***. At this point you can also modify which branch will have the tests run. Commit the change to your fork/develop.
-2. Sign up for Travis CI and allow access to your account. 
-3. Activate the repository within Travis CI.
-4. Modify the "README" file in the SU2/ directory such that the url points to the results for your fork rather than su2code/SU2.
-5. Commit the result into your fork/develop.
-6. View the results: when you open up your fork/develop on github the readme file should display. There will be a colored link going to the travis CI build which state whether the test is building, passing, or failing. This link will lead to the details of the tests. Pull requests between your fork/develop and any branches you have created on your fork will also run regression tests. 
-
-If the tests do not run at first, double check that the repository is activated within Travis CI, and if so push a commit with some small change to the travis.yml file to your repository. If it still doesn't work, double check your urls and refer to Travis CI help menu. 
diff --git a/_docs_v7/Style-Guide.md b/_docs_v7/Style-Guide.md
