-
Notifications
You must be signed in to change notification settings - Fork 9
Adding developer documentation #66
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
Conversation
1. **(completed)** Guidelines for contributing to OpenCMISS
2. **(completed)** Development Setup
3. **(placeholder)** Building OpenCMISS for developers
- The procedure for using the manage repo to build OpenCMISS from source (described in the [Further Details section of the existing Building OpenCMISS Libraries From Source](http://staging.opencmiss.org/documentation/building/index.html) needs to be moved here.
- The simpler procedure for getting started using the setup repo (i.e. for users) can be moved to a getting started section.
4. **(placeholder)** Coding Standard
- Need to add separate sections for Iron and Zinc.
- [Here is the link to coding style in Iron's doxygen documentation](https://github.com/OpenCMISS/cm/pull/172/files).
5. **(placeholder)** Documenting your code
- Among other things, we need to move or link the information in the 'Guide to contributing documentation' contained in the [CONTRIBUTE.md of the OpenCMISS/documentation repo ](https://github.com/OpenCMISS/documentation/blob/develop/CONTRIBUTE.md) here.
6. **(placeholder)** Submitting Code for Testing
- Need to add documentation on how to use Jenkins
7. **(completed)** Contributing
8. **(completed)** Review Process
9. **(placeholder)** API Documentation
- This section can be renamed 'technical documentation'. Need to add links to API docs that are currently in the [Technical documentation section of the OpenCMISS website](http://staging.opencmiss.org/documentation.html).
addresses issue OpenCMISS#65
| .. toctree:: | ||
| :maxdepth: 1 | ||
|
|
||
| dev_guidelines |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The style used in the heading of these rst documents are inconsistent. Some use sentence case, others use title case.
nickerso
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like a good starting point. would be good to get these changes up on staging so we have a consistent reference point for future discussion.
Updating GitHub issue descriptions on website based on pull request 66 reviewer comments.(OpenCMISS/documentation#66) Addresses issue OpenCMISS#65
|
Hi guys, Andre has reviewed/approved this pull request, hoping if we could pull it in into the develop branch. This will automatically trigger an update to the staging.opencmiss.org website. |
hsorby
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only part that I object to is the loss of the TL;DR section. I feel that having the commands first before any text is the best way to capture users who just want instructions.
|
Can add the TL;DR section in again in another pull request (but we probably need to define the term as some noobs like myself have no idea what that means and it becomes confusing). |
|
Actually might be better to add the TL;DR section to another page called Quick start. So that it does not confuse newbie developers. |
|
I really don't think we need to define the term, if people can't figure it out (and google is pretty useful for this sort of stuff) then they are going to have trouble using Iron and Zinc. |
|
We shouldn't have terms like TL;DR in the web documentation or indeed any other internet lingo, amusing or not. There is a significant academic side to the project and many people from grant reviewers and collaborators look at the site. The site should be able to pass the 60 yo professor reading it test. I'm afraid TL;DR does not really pass this test. |
|
I'm also against TL;DR and prefer 'Quick Start' or similar. |
| cmake --build . | ||
|
|
||
| The above instructions will not work if the basic requirements listed in :ref:`requirements` is not met. If executing these commands from a Visual Studio command prompt on a Windows machine the penultimate command may need to by adpated to include the generator required. For example if a 64 bit Visual Studio build is desired the command would be adapted to:: | ||
| If executing these commands from a Visual Studio command prompt on a Windows machine the penultimate command may need to by adpated to include the generator required. For example if a 64 bit Visual Studio build is desired the command would be adapted to:: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: --> adapted
| Building OpenCMISS for developers | ||
| ================================= | ||
|
|
||
| ** Coming soon** No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not put empty sections in. This harks back to the early web 'under construction' icons which are not seen any more and for good reason: they look bad and are completely useless right up until the work is actually done. The correct place for noting future work is a new issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added new issue for this #72
|
|
||
| In addition, a **Platform** label may be used to identify the issue as specific to a given platform (Windows/Linux/OS X). **Milestone** labels may be used to project when a feature is expected to be complete and/or indicate the priority of a given issue. Higher priority issues will take precedence and therefore be assigned a more immediate (lower) milestone number. | ||
|
|
||
| Topic Branch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consistently call them feature branches, or topic branches if there is consensus for that name. Github has excellent documentation and calls them feature branch; I much prefer feature branch over topic branch.
| git fetch prime develop | ||
| git checkout develop # Not required if already on develop branch | ||
| git merge prime/develop | ||
| git checkout -b issue46 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest we recommend users name their branches starting with the issue number followed by a word or words to succinctly describe it; 'issue' is redundant. Some examples I've used: 56_refactor_fe, 36_derivatives.
| .. Contributing Documentation | ||
| ===================== | ||
| Documenting your code |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this section. Put future tasks in separate issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue created for this #71
|
|
||
| .. The Documentation Builder link (entry 2. in step 5.) will take you to the build for the documentation. On this page you can see the steps taken to build the documentation. In the last step of the build (step 7.) there is a link 'dox' (entry 2.) that will take you to the built documentation. | ||
| **Coming soon: how to check documentation using Jenkins** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stuck record. Future work belongs in separate issues, not littering our documentation.
| - https://github.com/OpenCMISS/documentation | ||
| - https://github.com/OpenCMISS/website | ||
| - https://github.com/OpenCMISS/manage | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These links should appear as just 'iron', 'zinc' etc. and link to the address.
Add repo cmake_modules.
I think it would be good to present this as a table with a second column describing in a minimally brief statement what the repository contains.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A new issue has been created for this #73
|
|
||
| https://github.com/OpenCMISS | ||
|
|
||
| The OpenCMISS organisation contains the git repositories for each component of OpenCMISS: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
repositories for each of its components, documentation and tools:
|
|
||
| You now need to clone the selected repository to your PC. Do this by going to your fork (in this example user *prasad*'s fork) at:: | ||
|
|
||
| https://github.com/prasad/iron |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really don't like having an actual person's name here; keep credit in the git contribution record or a dedicated contributors page. Change 'prasad' to 'username' (which is what github calls it), and at the beginning define that it is to be substituted with your own username on github. You could alternatively write or emphasise it with italics.
|
|
||
| .. warning:: | ||
|
|
||
| Don't try and clone this location. Substitute your GitHub username for *prasad*. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is unnecessary if you said at the beginning that username stands for your user name.
| Now clone the repository:: | ||
|
|
||
| cd <somewhere/you/keep/development/code> | ||
| git clone git@github.com:prasad/iron.git |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should keep our story consistent with setup: recommend making a top opencmiss folder (I call mine oc to keep paths short) and in it add a sub folder src, and clone your fork of iron there. This is where the setup script will clone iron etc. (admittedly it clones the prime repo by default, but you can switch to your own remote).
Did you know if you subsequently run setup or manage on the same opencmiss (or 'oc') folder it will discover and keep your sources for any components in the src folder? It's very convenient for rebuilding from scratch. This is the convenient way to get your own forks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wondering if this would still work? Might be better if Hugh adds this to the build documentation.
| @@ -0,0 +1,5 @@ | |||
| ================= | |||
| API Documentation | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added an issue for this #75
| .. Instructions for testing OpenCMISS code | ||
| =========================== | ||
| Submitting Code for Testing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a new issue for this #74
| @@ -0,0 +1,100 @@ | |||
| ======================================== | |||
| Guidelines for OpenCMISS-Iron Developers | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing in here is actually specific to Iron, and it repeats substantial portions of content that is elsewhere. Merge all new content into the respective sections above and remove.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This page is not explicitly linked to the website. As you suggest, this page can be removed once the any relevant information has been removed. Created a new github issue for this: #77
| ============ | ||
|
|
||
| If an issue does not exist for the required work (e.g. implementation of a feature, fixing of a bug), then create a new one. The issue is the place to discuss the particulars related to the issue, discussions on determining the scope of the issue or clarification of any points that are unclear. | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add: Create the issue into issue list for the respective component repository it relates to. [What if it relates to multiple components? perhaps suggest creating an issue in each and adding a comment linking to the other e.g. cmlibs/zinc#60, as documented in github-secrets.]
This outline is outdated, see the latest outline on issue #65
This documentation has been built along with the OpenCMISS website and it builds fine with no errors. There are warnings regarding figure numbering, however, these will be addressed in another documentation issue as they are not critical.
(https://github.com/OpenCMISS/cm/pull/172/files).
Addresses issue #65