-
Notifications
You must be signed in to change notification settings - Fork 26
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
Adopt quotes around type names to make Reader ex. copy/paste-ready #340
Conversation
That code snippet certainly gets there magically in a way I don't understand. I did not realise the code was 'copied' over from npe2/_docs/templates/_npe2_readers_guide.md.jinja Lines 33 to 36 in 595b088
I note also that the types alias definitions used in annotation of the functions also magically get into the code snippet - BUT we are missing Maybe @melissawm or @psobolewskiPhD knows more about this? |
CI failures unrelated, don't worry about them, maybe due to the |
Fix for CIs in #341, we can update with main once that PR is merged. Thanks! |
This test is failing in CIs in #340. I think it is because there is no warning raised (not sure why the `UserWarning` is there). It used to pass because of this bug in pytest pytest-dev/pytest#9036, but this was fixed in pytest-dev/pytest#11129 and included in a recent release, thus our CIs are failing. Note that I have update the test to be more specific about the exception raised (`PackageNotFoundError`) but it is not needed, subclass exceptions are still accepted by pytest. Also added a message to be more specific. --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #340 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 37 37
Lines 2750 2750
=========================================
Hits 2750 2750 ☔ View full report in Codecov by Sentry. |
I understand the motivation, since the docs pluck out sub bits of But, if you go this direction, then you should also probably update
so, if someone were only looking at that module, they might say "hmm, why did they put them in quotes since they already imported it at the top of the module?" |
3de8178
to
ba1c285
Compare
indeed, good point @tlambert03 -- i update here: 3949b69 |
…mes to preempt reader confusion
ba1c285
to
3949b69
Compare
@tlambert03 I guess the CI depends on #344 for the moment? |
Looks like it |
@tlambert03 any idea how the type aliases at the top of the code snippet get there? It does not seem to be from the code block in the jinja template: npe2/_docs/templates/_npe2_readers_guide.md.jinja Lines 33 to 36 in 595b088
|
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! LGTM!
it is actually from that jinja block. that pipe ( Lines 125 to 130 in 595b088
which calls: Line 94 in 595b088
which grabs the needed types: Lines 117 to 122 in 595b088
which is defined here: Lines 79 to 91 in 595b088
|
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.
CI is green again after merging main. Thanks for these updates @vreuter ❤️
napari/docs#367 (review)
napari/docs#365