-
Notifications
You must be signed in to change notification settings - Fork 16
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.md #86
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -54,17 +54,60 @@ Please submit a [Pull Request][pulls] to submit a new term for consideration. | |||||||
|
||||||||
<a id="great-issues"></a> | ||||||||
|
||||||||
### How to write a great issue | ||||||||
|
||||||||
Please review GitHub's overview article, | ||||||||
["Tracking Your Work with Issues"][about-issues]. | ||||||||
|
||||||||
<a id="great-pulls"></a> | ||||||||
|
||||||||
### How to create a great pull/merge request | ||||||||
|
||||||||
Please review GitHub's article, ["About Pull Requests"][about-pulls], | ||||||||
and make your changes on a [new branch][about-branches]. | ||||||||
### GitHub Best Practice | ||||||||
|
||||||||
- Creating and curating issues | ||||||||
- Read ["About Issues"][[about-issues]] | ||||||||
- Issues should be focused and actionable | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||
- Complex issues should be broken down into simpler issues where possible | ||||||||
- Pull Requests | ||||||||
- Read ["About Pull Requests"][about-pulls] | ||||||||
- Read [GitHub Pull Requests: 10 Tips to Know](https://blog.mergify.com/github-pull-requests-10-tips-to-know/) | ||||||||
- Pull Requests (PRs) should be atomic and aim to close a single issue | ||||||||
- Long running PRs should be avoided where possible | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you mean "long"? "Long running" means persisting through time; that's not what you mean, do you? |
||||||||
- PRs should reference issues following standard conventions (e.g. “fixes #123”) | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. mention that this enables them to trigger actions like closing the linked issue? It's not just a style thing; it's functional. |
||||||||
- Schema developers should always be working on a single issue at any one time | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Really? Why schema developers in particular? |
||||||||
- Never work on the main branch, always work on an issue/feature branch | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This really depends on how the project is structured, and may be confusing to schema developers who aren't highly technical. |
||||||||
- Core developers can work on branches off origin rather than forks | ||||||||
- Always create a PR on a branch to maximize transparency of what you are doing | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Doesn't creating a PR in GitHub automatically make a branch? |
||||||||
- PRs should be reviewed and merged in a timely fashion by the {{cookiecutter.project_name}} technical leads | ||||||||
- PRs that do not pass GitHub actions should never be merged | ||||||||
- In the case of git conflicts, the contributor should try and resolve the conflict | ||||||||
- If a PR fails a GitHub action check, the contributor should try and resolve the issue in a timely fashion | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. switch lines 76 and 75 |
||||||||
|
||||||||
### Understanding LinkML | ||||||||
|
||||||||
Core developers should read the material on the [LinkML site](https://linkml.io/linkml), in particular: | ||||||||
|
||||||||
- [Overview](https://linkml.io/linkml/intro/overview.html) | ||||||||
- [Tutorial](https://linkml.io/linkml/intro/tutorial.html) | ||||||||
- [Schemas](https://linkml.io/linkml/schemas/index.html) | ||||||||
- [FAQ](https://linkml.io/linkml/faq/index.html) | ||||||||
|
||||||||
### Modeling Best Practice | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||
|
||||||||
- Follow Naming conventions | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||
- Standard LinkML naming conventions should be followed (UpperCamelCase for classes and enums, snake_case for slots) | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||
- Know how to use the LinkML linter to check style and conventions | ||||||||
- The names for classes should be nouns or noun-phrases: Person, GenomeAnnotation, Address, Sample | ||||||||
- Spell out abbreviations and short forms, except where this goes against convention (e.g. do not spell out DNA) | ||||||||
- Elements that are imported from outside (e.g. schema.org) need not follow the same naming conventions | ||||||||
- Multivalued slots should be named as plurals | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you add a brief example? Do you mean, like, "synonyms"? |
||||||||
- Document model elements | ||||||||
- All model elements should have documentation (descriptions) and other textual annotations (e.g. comments, notes) | ||||||||
- Textual annotations on classes, slots and enumerations should be written with minimal jargon, clear grammar and no misspellings | ||||||||
- Include examples and counter-examples (intentionally invalid examples) | ||||||||
- Rationale: these serve as documentation and unit tests | ||||||||
- These will be used by the automated test suite | ||||||||
- All elements of the nmdc-schema must be illustrated with valid and invalid data examples in src/data. New schema elements will not be merged into the main branch until examples are provided | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||
- Invalid example data files should be invalid for one single reason, which should be reflected in the filename. It should be possible to render the invalid example files valid by addressing that single fault. | ||||||||
- Use enums for categorical values | ||||||||
- Rationale: Open-ended string ranges encourage multiple values to represent the same entity, like “water”, “H2O” and “HOH” | ||||||||
- Any slot whose values could be constrained to a finite set should use an Enum | ||||||||
- Non-categorical values, e.g. descriptive fields like `name` or `description` fall outside of this. | ||||||||
- Reuse | ||||||||
- Existing scheme elements should be reused where appropriate, rather than making duplicative elements | ||||||||
- More specific classes can be created by refinining classes using inheritance (`is_a`) | ||||||||
|
||||||||
[about-branches]: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/about-branches | ||||||||
[about-issues]: https://docs.github.com/en/issues/tracking-your-work-with-issues/about-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.
I assume ["About Issues"][[about-issues]] should be ["About Issues"][about-issues]