Closed jwallwork23 closed 2 months ago
jwallwork23
commented
2 months ago Thanks for your reviews @erizmr @ddundo, will address later.
On second thoughts, does anyone have a preference on whether the dev docs information should live in the wiki or on the website? I thought about this because I realised its not as easy to review wiki changes because they don't appear in the "Files changed" pane.
ddundo
commented
2 months ago I like the idea of having everything on the website (including the installation instructions), but I don't have a strong preference :)
But either way I think that we should add a link to this "contributing" page to animate/goalie/movement readme's. At the moment it's hard to find.
jwallwork23
commented
2 months ago LGTM. I read the Development Practices and it is very instructive! One point I come up when reading the Pull Requests part is that do we expect contributors to work on branches in the main repo directly or their forked repos? Which do you think is better?
@erizmr It's more a matter of whether you have permission to create a branch. Added "Note that if you are not a member of the mesh-adaptation organisation then you will have to fork the repository that you wish to contribute to."
acse-ej321
commented
2 months ago @jwallwork23 - some comments on the wiki Development Practices page:
You could generalise the scope by changing the first sentence on the wiki to something like “This page documents highly recommended practices for contributions to the affiliated Mesh Adaptation modules.”
Under ‘Projects’>’Mesh Adaptation development board’: o As we have already discussed adding categories, you might just say “The project board categories are:” as the lead in for the list, and, instead of sequentially numbering the categories, list them with the category in bold. That might allow for easier editing. o “Notice that all five categories refer to Issues. Pull Requests should not be put on the project board.” – (1) you could add hyperlinks from Issues and Pull Requests to the related sections in the wiki. (2) you could just say ‘all the categories refer to Issues’ to allow for simpler edits if the total number changes.
Under ‘Issues’: o It would be good if the ‘New Issues links: Animate, Goalie, Movement” and ‘Mesh Adaptation development project board’ links opened up automatically in a new tabs so one still had the Development Practices open to refer back to.
Under ‘Pull Requests’:
o Item 4: You have a note and a link to a note. The link doesn’t seem to work. However you could just move the contents of the note into the body of item 4 where it is directly referenced?
o Item 5: there is a missing closing parenthesis for “(See the Testing section for more details.”
o Item 11: (1) the link to ‘Test Suite’ doesn’t seem to work
o It would be good if the ‘New PR links: Animate, Goalie, Movement” opened up automatically in a new tabs so one still had the Development Practices open to refer back to.
o Item 12: (1) the link to ‘below’ doesn’t seem to work
o Item 13: In item 3, you provide the CL Git code for creating a new branch, but don’t subsequently provide the CL Git code here for deleting the branch. Maybe split Item 13 into Item 13 – referencing the GitHub steps and a new Item 14 – “Command line: Remember to delete your local copy of the branch, too: using git -d branch XX_<branch_name>
Under ‘Review Process’: o You don’t really need the extra parentheses around “(who wrote the code in the PR)” and “(who have been assigned to review the PR). o It would be good to add a hyper link to “point 9” o For consistency you might format ‘the reviewer(s)’ in bold in the sentence The recommended workflow for the reviewer(s) to follow is as follows: “
Under ‘Testing’:
o Maybe shorten: “Animate, Goalie, and Movement each have dedicated test suites. All three have a subdirectory called test containing a mixture of unit tests and integration tests.” -> “All Mesh Adaptation codes have a subdirectory called test containing dedicated test suites with a mixture of unit tests and integration tests.”
o This is a bit confusing: “Goalie also has a test_adjoint subdirectory, which includes tests for functionality making use of adjoint methods. (More accurately, the test subdirectory of Goalie includes test which do not make use of the adjoint, i.e., annotation is not turned on.)” Maybe something like “Goalie also has an additional test_adjoint subdirectory, specifically for tests of functionality making use of adjoint methods where annotation is turned on. All tests which do not make use of the adjoint, i.e., annotation is not turned on, should be located under the standard test subdirectory”
o Add hyperlinks to ‘unit tests’ and ‘integration tests’?
Under ‘Formatting’: o “Animate, Goalie, and Movement use Ruff for linting and formatting.” Could be generalised to “The Mesh Adaptation codes use Ruff for linting and formatting.” o ‘Pre-commit hook’: “Upon making any commit to Animate, Goalie, or Movement, the pre-commit hook will be invoked and will use Ruff to format the code.” Could be generalised to : “Upon making any commit to a Mesh Adaptation module, the pre-commit hook will be invoked and will use Ruff to format the code.”
Under ‘Docker build’: o “The Mesh Adaptation Docs repository” – can this be linked here? o The link ‘installation instruction page.” does not see to work.
jwallwork23
commented
2 months ago Thank you @acse-ej321 for your very thorough review! This is great. I think I've addressed most of your concerns, except the following.
Regarding opening links in new tabs, it doesn't appear to be possible in GitHub. I tried this (inspect the source to see my HTML) but it didn't work. (See https://stackoverflow.com/questions/41915571/open-link-in-new-tab-with-github-markdown-using-target-blank)
It would be good to add a hyper link to “point 9”
I don't know if this is possible?
Closes #8. Closes #11.
The code changes in this PR are minimal. However, as part of reviewing this PR it would be great if people could have a read of the Development Practices wiki page that I added. Hopefully it reads well and is instructive. Feedback welcome!