-
-
Notifications
You must be signed in to change notification settings - Fork 157
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 guide #2382
Conversation
Going to go ahead and mark this ready for review because the build passed locally with the newer changes. @Misty-W @natestemen @nathanshammah I tried to think of all the things a person has to keep track of when adding a new method or a tutorial. Let me know if I forgot something! |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2382 +/- ##
==========================================
- Coverage 98.32% 98.31% -0.01%
==========================================
Files 87 87
Lines 4059 4042 -17
==========================================
- Hits 3991 3974 -17
Misses 68 68 ☔ View full report in Codecov by Sentry. |
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.
Nice updates! Looking good to me.
One question: is it possible to use local links to markdown links instead of using mitiq.readthedocs.io
links? I think it makes browsing the docs locally a little easier, and if it works then double win!
I am having some trouble with directly referencing For example, in a line of I have also tried using FWIW I have also verified I am using the correct heading anchors through I could ignore these warnings through |
Feel free to disregard my previous comment. I should have used |
@natestemen I am thinking of replacing Line 8 in 837ea83
I feel the statement
Also, do you happen to know why the mitiq/docs/source/toc_contributing.md Lines 1 to 8 in 837ea83
Then, what's the use of having |
Definitely the latter. There are no stupid questions. Modifying it to make that point clearer is a good idea. Since the link is to the discussion page where the FAQ is pinned, I don't think we need to add an additional link here.
It's considered "best practice" by many to have a few things at the root of a repository. Usually you'll see a
Yeah so these files just exist to get |
That's pretty weird. I found this stack overflow answer where the best practice of placing such files in the root of the repo has since changed to using |
this PR is ready |
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.
Thanks Purva! Looking good. Just some wordsmithing comments, nothing major.
CONTRIBUTING.md
Outdated
## Code of conduct | ||
Mitiq development abides to the [Contributors' Covenant](https://mitiq.readthedocs.io/en/latest/code_of_conduct.html). | ||
Mitiq development abides to the [](./code_of_conduct.md#contributor-covenant-code-of-conduct). |
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.
Unnecessary since the anchor is the title.
Mitiq development abides to the [](./code_of_conduct.md#contributor-covenant-code-of-conduct). | |
Mitiq development abides to the [](./code_of_conduct.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.
This was not working when I was checking the changes locally. The link was dead.
I had to make sure the main heading was specified after .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.
What error are you seeing? I just tried locally and it's working!
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.
No error. The line appears as non-clickable as if we sandwiched the path of the md file in backticks.
If it works for you, I could give it a try and check it in the RTD build.
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 to me (same as what I see locally): https://mitiq--2382.org.readthedocs.build/en/2382/contributing.html#code-of-conduct
docs/CONTRIBUTING_DOCS.md
Outdated
|
||
Information in the docs should be added as markdown files using the MyST markdown syntax. | ||
If you are adding a new file (as opposed to editing an existing one), ensure to add it to an associated TOC so that it is discoverable. | ||
If you are adding a new file (as opposed to editing an existing one), ensure to add it to an associated TOC so that it is discoverable. |
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.
If you are adding a new file (as opposed to editing an existing one), ensure to add it to an associated TOC so that it is discoverable. | |
If you are adding a new file (as opposed to editing an existing one), ensure to add it to an associated TOC so that it is discoverable. |
We should set up a linter for this.
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.
If you use make docs
without adding the new docs addition to the TOC, sphinx will complain that the file is not added anywhere. Do we still want to add a linter?
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.
Sorry, I mean we should set up a linter to catch white space at the end of lines.
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.
Ah. Will have to search for something.
I see there's a sphinx extension for it but a quick glance shows it is for .rst
files only: https://github.com/sphinx-contrib/sphinx-lint
There is a [sample markdown formatted notebook in the `examples` directory](./examples/template.md) for you to take a look at as you write your own! | ||
There is a [sample markdown formatted notebook in the `examples` directory](../source/examples/template.md) for you to take a look at as you write your own! |
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'm confused by this. It seems both main
and this branch both work here. Any idea why that is? Is the path with source
preferred?
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.
Same as my other comment. I had to make sure I specified the full path. The link would not be live otherwise (locally).
I thin this is because CONTRIBUTING.md
and CONTRIBUTING_DOCS.md
files are in separate directories compared to the rest of the files in docs/
One general thing: make sure to have a PR description with... well a description of what the PR contains. Maybe some motivation/etc. This is especially helpful when looking back through the git history and trying to understand why things were changed. |
Co-authored-by: nate stemen <nate@unitary.fund>
Co-authored-by: nate stemen <nate@unitary.fund>
* update both guides * forgot readme * move things around * inline link cleanup * Nate's review - corrected typos Co-authored-by: nate stemen <nate@unitary.fund> * Apply suggestions from code review Co-authored-by: nate stemen <nate@unitary.fund> --------- Co-authored-by: nate stemen <nate@unitary.fund>
Description
Updates the instructions for contributors in the documentation.
Code of conduct
was removed fromcontributing.md
.md
links through cross-referencingcontributing_docs.md
.License
Before opening the PR, please ensure you have completed the following where appropriate.