Update CONTRIBUTING.md #86
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Adding some general guidelines abstracted from NMDC
nlharris
approved these changes
Dec 8, 2023
| ### GitHub Best Practice | ||
|
|
||
| - Creating and curating issues | ||
| - Read ["About Issues"][[about-issues]] |
There was a problem hiding this comment.
I assume ["About Issues"][[about-issues]] should be ["About Issues"][about-issues]
| - 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.
Do you mean "long"? "Long running" means persisting through time; that's not what you mean, do you?
| - 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 | ||
| - PRs should reference issues following standard conventions (e.g. “fixes #123”) |
There was a problem hiding this comment.
mention that this enables them to trigger actions like closing the linked issue? It's not just a style thing; it's functional.
| - Pull Requests (PRs) should be atomic and aim to close a single issue | ||
| - Long running PRs should be avoided where possible | ||
| - PRs should reference issues following standard conventions (e.g. “fixes #123”) | ||
| - Schema developers should always be working on a single issue at any one time |
There was a problem hiding this comment.
Really? Why schema developers in particular?
| - Long running PRs should be avoided where possible | ||
| - PRs should reference issues following standard conventions (e.g. “fixes #123”) | ||
| - Schema developers should always be working on a single issue at any one time | ||
| - Never work on the main branch, always work on an issue/feature branch |
There was a problem hiding this comment.
This really depends on how the project is structured, and may be confusing to schema developers who aren't highly technical.
| ### Modeling Best Practice | ||
|
|
||
| - Follow Naming conventions | ||
| - Standard LinkML naming conventions should be followed (UpperCamelCase for classes and enums, snake_case for slots) |
There was a problem hiding this comment.
Suggested change
| - Standard LinkML naming conventions should be followed (UpperCamelCase for classes and enums, snake_case for slots) | |
| - Follow standard LinkML naming conventions (UpperCamelCase for classes and enums, snake_case for slots) | |
| - [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.
Suggested change
| ### Modeling Best Practice | |
| ### Data Modeling Best Practice | |
| - 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.
can you add a brief example? Do you mean, like, "synonyms"?
| - 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.
Suggested change
| - 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 | |
| - All elements of the schema should be illustrated with valid and invalid data examples in src/data. | |
|
|
||
| - Creating and curating issues | ||
| - Read ["About Issues"][[about-issues]] | ||
| - Issues should be focused and actionable |
There was a problem hiding this comment.
Suggested change
| - Issues should be focused and actionable | |
| - Issues should be focused and actionable. Give an example of the bad behavior and describe how it should have behaved. | |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adding some general guidelines abstracted from NMDC