-
Notifications
You must be signed in to change notification settings - Fork 116
ci: Add doc-test #361
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
ci: Add doc-test #361
Conversation
75eb49b
to
36f8d18
Compare
c76d0cd
to
c2391b0
Compare
@lan496 @atztogo I need your feedback about 00255f1. Do you prefer a different form for the prefix? Also all of the Other than that I could use the help with the Oh, and also, we need review of the spellcheck |
3657897
to
00255f1
Compare
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 have no preference. LGTM.
What should we do? |
As for
|
|
(Note for me)
|
I think we should address these issues here. Check e93df28 for how we can address them. There are a few issues present:
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #361 +/- ##
========================================
Coverage 83.86% 83.86%
========================================
Files 24 24
Lines 8180 8180
========================================
Hits 6860 6860
Misses 1320 1320
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
0ce0edf
to
ce9c798
Compare
I have addressed all of the linkcheck and citation issues. The only remaining issue is the spelling, and I think we should discuss and fix them here. Most of them I have made exceptions, the remaining ones, please review and address, e.g. |
Thanks! The Anyway, these words should be added to the wordlist.
Let's use |
I don't think a blacklist linter would make sense because there are infinite ways that words can be misspelled. On the other hand, the whilelist tells it that someone has reviewed and made a decision on what the style-guide is.
Indeed this one is a one-time difficulty, which we have to address. I don't like having warnings without addressing them, just like the About the other spellings I want to clarify a few things:
|
I am concerned that the whitelist for
Indeed. I totally agree that it is not a decisive solution. Word definitions
|
Indeed, that is also why it is only on the GitHub CI and not on pre-commit. It should not be considered as a hard barrier for the writer to check, but I think it is useful to have it for the reviewer. E.g. in this case there are some words that are niche even to the first-principles community, in which case I think it is helpful to link the reader to the external definition.
Oh, good call, I will look more carefully in the context of these.
Great, I can handle these with that information. I'll get back to this on the weekend. |
Thanks for pointing out this, @LecrisUT and @lan496. This is a typo. This will be changed to k-points. All q-point(s) should be also replaced by k-points(s). The purpose to develop Kpoints related functions was to use them for my phonon calculation. In the old time, I never though that spglib would become so popular, so I was not careful about it, and so I could write such the confusing documentation. Now I want to prepare another library specific for reciprocal space mesh, and made those functions in spglib deprecated at future version, because the grid generated by current mesh setting can break crystallographic point group. I wrote those theoretical detail in the paper, https://iopscience.iop.org/article/10.1088/1361-648X/acd831. |
I see. I am frighted the default For terminology in crystallography, it will be nice to add all words in IUCr's dictionary. I'll prepare it. |
I would say, we don't need to proactively add all dictionary words, it is trivial to add them as they become relevant. The names is indeed unfortunate, and I don't know of good ways to add them. Contributor names should automatically be handled at least (I think taken from git? Not sure how that works). A small silver-lining to this, at least we would not be misspelling people's names :D |
Looks like taken from git history, https://sphinxcontrib-spelling.readthedocs.io/en/latest/customize.html#word-filters. Oh, I just collect words from the page (never mind it's an easy scraping job). word list
|
Well, as you already have worked on this convincing PR, I am OK to proceed in this direction. Let me think again if the new linters cause confusion on possible contributors. |
Cool, thanks for checking. Make sure you don't have a typo in your git author :D
It's a bit nuanced because it doesn't seem to pick up on multiple words. I have tracked down that the |
For now I have dropped |
@LecrisUT Can you rebase develop branch? Recently, I've changed python API docs a lot. |
This one has already been rebased to include them, I'll do it again after the F38 PR |
I guess #393 is still not reflected. |
Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
Still haven't figured how to convince github that the commits are already present, maybe with merged commits 🤷 |
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.
LGTM!
Run a few sphinx doc tests:
html
with-W
make warnings tests errorlinkcheck
check if we are linking to any dead linksdoctest
run the snippets and check the output resultMissing a good spell-checkerspelling
check English and consistency spellingsCloses #359