-
Notifications
You must be signed in to change notification settings - Fork 23
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
Update CONTRIBUTING and README #1808 #1937
Conversation
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 good and it seems to be much more readable now, thanks!
@LillyG901 please take a look at the Reviewer Guide and review this PR as 2nd reviewer. |
Co-authored-by: stap-m <38690039+stap-m@users.noreply.github.com>
as suggested Co-authored-by: Colin <colinheidfeld@gmail.com>
Co-authored-by: Colin <colinheidfeld@gmail.com>
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.
Apart from that typo: Looks good to me :)
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.
Found no semantic issues.
All comments refer to small grammar mistakes or sections that were hard to comprehend for me during my first read through.
They may be ignored, if you don't consider them to be useful changes to the files.
- [Use Protégé to Change the Ontology](https://github.com/OpenEnergyPlatform/ontology/wiki/How-to-use-prot%C3%A9g%C3%A9-to-change-the-ontology) | ||
- [Term Tracker Annotation](https://github.com/OpenEnergyPlatform/ontology/wiki/Term-Tracker-Annotation) | ||
<br> | ||
... as well as the [CONTRIBUTING.md](https://github.com/OpenEnergyPlatform/ontology/blob/dev/CONTRIBUTING.md). |
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.
When vieved in the wiki Contributing.md does not show up as link(CONTRIBUTING.md), but rather as [CONTRIBUTING.md] (https://github.com/OpenEnergyPlatform/ontology/blob/dev/CONTRIBUTING.md).
I'm unsure what causes this, as copying the line above to show the issue in this comment did not reproduce the issue,
|
||
| Emoji ... | ... symbolizes tool ... | ...which is used for ... | | ||
|:------------------------:|:-----------------------:| ---------------------------------------------------------- | | ||
| 🔶 <br/>*orange diamond* | git | keeping code in sync between your PC and online-repository | | ||
| 🐙 <br/>*octopus* | github | discussions and reviews | | ||
| 📙 <br/>*orange book* | protégé | changing the ontology | | ||
| 📝 <br/>*memo* | text editor | changing the ontology as well as other files | | ||
| 📝 <br/>*memo* | text editor | changing the ontology as well as other files |
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.
Line does not end in | as all prev lines.
This does not affect the table, when viewed on the wiki.
|
||
**short-description** | ||
|
||
Describe shortly what the branch is about. Avoid long and short descriptive names for branches, 2-4 words are optimal. |
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.
"Avoid long and short descriptive names for branches" sounds unintuitive to me. Like a description might be both short and long at the same time.
I'd change it to "Avoid descriptive names for branches that are either too short or too long".
|
||
Other hints: | ||
- Separate words with `-` (minus) | ||
- Do not put your name to the branch name, it's a collaborative project |
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 your name to the branch name" is grammatically incorrect, if I'm not mistaken,
Change either to "Do not put your name in the branch name" or to "Do not add your name to the branch name".
``` | ||
to edit the commit message of your latest commit (provided it is not already pushed on the remote server). <br> | ||
With `--amend` you can even add/modify changes to the commit. | ||
| |
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 assume this is the missing "|" of line 40.
It seems out of place to me.
@@ -39,9 +114,12 @@ by [Vincent Driessen](https://nvie.com/posts/a-successful-git-branching-model/). | |||
|
|||
[Assign a project](https://help.github.com/en/github/managing-your-work-on-github/adding-issues-and-pull-requests-to-a-project-board#adding-issues-and-pull-requests-to-a-project-board-from-the-sidebar) (default "Issues") to the issue | |||
|
|||
Discussion about the implementation should take place within the issue. **Important**: Please discuss the proposal within the issue thread **before** starting to work on a solution. For minor changes, which include small changes to improve clarity of definitions and the addition of clarifying annotations, at least one other person from the project team should agree to the proposed change before it is implemented. For major changes, which include adding new entities and restructuring the ontology, at least two other members of the project team need to agree to the change before it is implemented, which should include at least one domain expert and at least one ontology expert. Issues which are contentious, for which it is difficult to reach agreement, should be added to the agenda of the next ontology working group teleconference for discussion to reach agreement amongst the full working group. Subsequent to such discussion, the issue's first thread should be updated with a documented record of the conclusions reached. | |||
Discussion about the implementation should take place within the issue. **Important**: Please discuss the proposal within the issue thread **before** starting to work on a solution. <br> |
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.
Suggestion:
Add another line break before Important to make it stand out more while reading.
The boldness alone is not as noticeable.
@@ -85,11 +163,12 @@ by [Vincent Driessen](https://nvie.com/posts/a-successful-git-branching-model/). | |||
``` | |||
If your branch does not exist on the remote server yet, git will provide you with instructions, simply follow them. | |||
|
|||
Hint: You can create a draft pull request directly after your first commit 🐙, see 7.). Then you get the pull request number and 📙 implement the [term tracker items](https://github.com/OpenEnergyPlatform/ontology/wiki/term-tracker-item) in Protégé. Only after finishing the implementations you can assign reviewers and thus change the state of the PR. Using that workflow, it is clear whether a PR is actually ready for review. | |||
**Hint:** You can create a draft pull request directly after your first commit 🐙, see 7.). <br> |
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.
", see 7.)." is hard to read for me.
I suggest changing it to "(see 7).".
@LillyG901 |
Alright sounds good to me :) |
Summary of the discussion
Made changes to CONTRIBUTING.md and README.md in order to make it more accessable to new contributors. See Issue #1808
Type of change (CHANGELOG.md)
Update
Workflow checklist
Automation
Closes #1808
PR-Assignee
term tracker item
(this doesn't apply as no changes are made to the oeo)Reviewer