-
Notifications
You must be signed in to change notification settings - Fork 360
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
Docs and test refactoring #552
Conversation
Wieee!!!! I appreciate that you do this chore work, as even reviewing markdown is preferable to me if additions are made later :) Could you split apart this PR in two, where tests are relocated separately? I'm not feeling confident we can do that, but if we can and its okay, then I think its relevant to have in a dedicated PR to help highlight that in the changelog because its a possibly breaking change for people that relies on the tests, fixtures, or similar. I'll come back and review the docs section as soon as I've completed the review on another PR! |
```{eval-rst} | ||
.. literalinclude:: example-oauthenticator.py | ||
``` |
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 think this can be written directly in MyST markup. I parse the rST code to be invoking a sphinx directive, and that would match in MyST markup as described here: https://myst-parser.readthedocs.io/en/latest/syntax/roles-and-directives.html
So it would become...
```{literalinclude} example-oauthenticator.py
```
Or perhaps this works?
```{literalinclude} example-oauthenticator.py```
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!! The docs parts LGTM and is ready to merge following review comments resolves, but I'm cautious to merging the test folder relocation as part of this PR as it could even be a breaking change. I hope you feel alright with splitting this PR in two!
EDIT: I opened jupyterhub/team-compass#588 to get further input on the topic of relocating tests.
56ee8c9
to
953790f
Compare
Co-authored-by: Erik Sundell <erik.i.sundell@gmail.com>
for more information, see https://pre-commit.ci
Co-authored-by: Erik Sundell <erik.i.sundell@gmail.com>
Co-authored-by: Erik Sundell <erik.i.sundell@gmail.com>
Co-authored-by: Erik Sundell <erik.i.sundell@gmail.com>
Haha, something strange happened. I've squashed the last commits, and I could see them squashed in the branch main...GeorgianaElena:oauthenticator:docs-refactoring, but not in this PR. So I closed it, hoping to reopen it after and all to be fixed and updated. But now the reopen button is deactivated |
If you force push to a branch and the PR is closed it'll be disconnected (I guess GitHub assumes a force push means you've reused the branch for something else?) |
Very good thing to know 😅
Maybe 🤷🏼♀️ The other mystery thing is why the forced-pushed changes (commits squashing) didn't show up in the PR although everything seemed correct when checking out the actual branch. |
Before #549, I believe some maintenance work needs to happen.
This PR:
repo_root/oauthenticator/tests
torepo_root/tests
(to be in line with other jupyterhub repos).rst
files to.md
files and hopefully much of the old syntax with myst markdown oneNote
The docs content was not changed more than a few typos I spotted here and there, to not bloat this PR more.