Update docs for "add_additional_resource" and other fixes #233
Conversation
* Add hepdata_lib.c_file_reader and hepdata_lib.helpers modules. * Pin Sphinx<7 since docs fail to be built with Sphinx v7. * Replace m2r2 with sphinx-mdinclude to avoid Mistune conflict. * Replace deprecated "python setup.py test" with "pytest tests". * Add section to "Developer information" about building docs. * Add sections to "Usage" on "Adding resource links or files". * Mention that venv can be used instead of virtualenv/virtualenvwrapper.
* Allow for type="HistFactory" to be passed as an additional argument. * Extend test_submission.py to test the additional "type" argument. * Add newlines to docstring of round_value_and_uncertainty for Sphinx.
* Installation needed to build the module index. * Add "fail_on_warning: true" to avoid building broken docs.
* Links to Jupyter notebooks previously broken on "Read the Docs". * Binder links need to be given on a separate line to work properly. * Add link to "Read the Docs" so it appears on main GitHub repo page.
* Needed to install test dependencies with: pip install -e ".[test]" * Remove deprecated "setup_requires" and "tests_require". * Add Python v3.11 to "Classifiers".
* Python v3.11 incompatible with pylint v2.9.6 (installs wrapt v1.12.1). * Add upper pin python_requires='>=3.6,<3.11' in setup.py. * Use Python v3.10 in "Read the Docs" configuration file.
* Change "type" to "file_type" in argument of add_additional_resource. * Disable some messages from Pylint when run on my local installation.
Codecov Report
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. @@ Coverage Diff @@
## main #233 +/- ##
==========================================
+ Coverage 88.52% 88.54% +0.02%
==========================================
Files 4 4
Lines 976 978 +2
Branches 202 203 +1
==========================================
+ Hits 864 866 +2
Misses 82 82
Partials 30 30
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
@GraemeWatt I'm about to go on holiday for a week and I won't have internet access, so while I might be able to do a first pass on this before I go to bed tonight I think I'll defer everything to your judgement and @clelange review. Thanks for this PR! |
Sorry, I meant to comment that your review would be optional and only if you had spare time. I just thought I'd add you as a Reviewer since you contributed to improving this repository earlier in the year. |
There was a problem hiding this comment.
Thanks @GraemeWatt! This all looks good with the exception of the cap on requires-python metadata. This review comes from plane WiFi and the GitHub mobile app so I apologize for not giving better inline suggestions or clarity.
setup.py
Outdated
| @@ -37,14 +37,17 @@ | |||
| keywords='HEPData physics OpenData', | |||
| packages=find_packages(exclude=['contrib', 'docs', 'tests']), | |||
| zip_safe=False, | |||
| python_requires='>=3.6', | |||
| python_requires='>=3.6,<3.11', | |||
There was a problem hiding this comment.
Capping Python requires should be avoided for reasons given by Henry here: https://iscinumpy.dev/post/bound-version-constraints/
There was a problem hiding this comment.
Thanks for the feedback and your dedication to prompt PR reviewing! Enjoy your vacation!
I agree to remove the cap <3.11. My reasoning was that installation should not be allowed for Python versions not included in the test matrix. However, in this case, I checked locally that the tests pass with Python 3.11. It's only Pylint that causes problems with Python 3.11, so I've opened a new issue #234.
docs/setup.rst
Outdated
| @@ -47,7 +47,7 @@ If you would like to develop the code, you need to install the package from the | |||
| :: | |||
|
|
|||
| cd $SOMEPATH | |||
| git clone git@github.com:HEPData/hepdata_lib.git | |||
| git clone https://github.com/HEPData/hepdata_lib.git | |||
There was a problem hiding this comment.
Switching to HTTPS is fine, though I like poking people towards using SSH when possible.
There was a problem hiding this comment.
I guess it's personal preference whether to use HTTPS or SSH, so I've given both URLs and linked to the GitHub documentation. I just changed to HTTPS because that's what I use myself.
There was a problem hiding this comment.
I'd also prefer to keep SSH here
There was a problem hiding this comment.
Hi @GraemeWatt - thanks, this changes quite a lot, but I agree with @matthewfeickert that things overall look good. I left a couple of suggestions/comments in the code.
Two general questions:
- Why did you change the formatting in so many places, in particular those lines/separators?
- Do we really need to disable pylint for missing imports in so many places (
# pylint: disable-msg=E0401) or is this a setup issue on your side?
README.md
Outdated
| - [Reading in ROOT histograms](examples/reading_histograms.ipynb) [](https://mybinder.org/v2/gh/HEPData/hepdata_lib/master?filepath=examples/reading_histograms.ipynb) | ||
| - [Reading a correlation matrix](examples/correlation.ipynb) [](https://mybinder.org/v2/gh/HEPData/hepdata_lib/master?filepath=examples/correlation.ipynb) | ||
| - [Reading TGraph and TGraphError from '.C' files](examples/read_c_file.ipynb) [](https://mybinder.org/v2/gh/HEPData/hepdata_lib/master?filepath=examples/read_c_file.ipynb) | ||
| - [Reading in text files](https://github.com/HEPData/hepdata_lib/blob/master/examples/Getting_started.ipynb)\ |
There was a problem hiding this comment.
What's the reason you're changing this? Relative links would also work in branches whereas here you hard-code them to master. I'd prefer to keep things as they are.
There was a problem hiding this comment.
The problem is that the README.md file is rendered in two different places. Firstly, on the main page of the repo (https://github.com/HEPData/hepdata_lib#further-examples) where it displays fine. But the README.md file is also converted to RST format for rendering on Read the Docs (https://hepdata-lib.readthedocs.io/en/latest/README.html#further-examples). This is currently broken. The relative links do not work giving "404 Not Found" from Read the Docs. The Binder images/links also do not render properly. The Read the Docs build gives error/warning messages like:
../README.md:96: ERROR: Unexpected indentation.
../README.md:101: ERROR: Unexpected indentation.
../README.md:106: ERROR: Unexpected indentation.
../README.md:111: ERROR: Unexpected indentation.
../README.md:116: ERROR: Unexpected indentation.
/home/docs/checkouts/readthedocs.org/user_builds/hepdata-lib/checkouts/stable/docs/contributing.rst:75: WARNING: Title underline too short.
These messages are also apparent when running Sphinx locally. My fixes are a compromise to get rid of these error messages and ensure an acceptable rendering on both GitHub and Read the Docs. Admittedly, the spacing now looks worse than before on GitHub, but it was the best I could do. Now I've enabled fail_on_warning: true in the Read the Docs configuration, such error messages will cause the build to fail rather than being hidden away.
There was a problem hiding this comment.
I tried to improve the layout of the "Further examples" section in my latest commit. Now the Binder images/links appear on the same line as the text on GitHub (as before). On Read the Docs, the Binder images/links appear on the line below the text. The two line breaks <br/><br/> are needed to increase the separation after each bullet point because otherwise the Binder images/links appear immediately above the text of the next bullet point.
docs/setup.rst
Outdated
| @@ -47,7 +47,7 @@ If you would like to develop the code, you need to install the package from the | |||
| :: | |||
|
|
|||
| cd $SOMEPATH | |||
| git clone git@github.com:HEPData/hepdata_lib.git | |||
| git clone https://github.com/HEPData/hepdata_lib.git | |||
There was a problem hiding this comment.
I'd also prefer to keep SSH here
docs/setup.rst
Outdated
|
|
||
| :: | ||
|
|
||
| cd $SOMEPATH | ||
| git clone [email protected]:HEPData/hepdata_lib.git | ||
| git clone https://github.com/HEPData/hepdata_lib.git | ||
| OR git clone [email protected]:HEPData/hepdata_lib.git |
There was a problem hiding this comment.
... and then make the HTTPS option a comment so one can copy and paste
There was a problem hiding this comment.
OK, I've reverted to giving SSH, with the HTTPS option given in the text above. Personally, I just find it easier to use HTTPS since it doesn't require an SSH keypair, especially if I just want to checkout a repository without pushing to it.
docs/setup.rst
Outdated
| @@ -92,6 +98,15 @@ You can always activate the virtual environment in another shell by calling the | |||
| workon hepdata_git | |||
| python myscript.py # Execute script using development branch | |||
|
|
|||
| Alternatively, if you don't want to install the additional virtualenv_ and virtualenvwrapper_ packages, you can simply | |||
There was a problem hiding this comment.
Since we're on this topic, I would actually prefer to have this the default since most people will not have virtualenv or virtualenvwrapper available and it will be confusing to state things that are using non-standard tools first. Maybe we should get rid of the virtualenv and virtualenvwrapper section altogether.
There was a problem hiding this comment.
Agree, I think the text on virtualenv/virtualenvwrapper is a hangover from Python 2 where the venv module was not available. I've rewritten the text to use the venv module instead.
* Renamed 'master' branch to 'main' as with other HEPData repositories. * Upgrade actions in GitHub Actions workflows to close Dependabot PRs. * Revert to an SSH URL for main "git clone" with HTTPS URL in text. * Remove recommendation to use virtualenv/virtualenvwrapper not 'venv'.
* Remove 'disable-msg' comments introduced in commit dfa0a89. * Add empty "__init__.py" file to tests/ directory. * Add 'math' and 'array' modules to extension-pkg-whitelist in pylintrc. * test_utilities.py needs to be a relative import after hepdata_lib. * With these changes, pylint now gives 10.00/10 on my local setup.
Thanks for the useful review! I've commented separately on your individual points. The formatting changes were necessary to avoid errors/warnings about incorrectly formatted I also renamed the Is the |
* Tweak "Further examples" so Binder on same line as text on GitHub. * Remove pin on docutils==0.19, as not needed in recent Sphinx versions. * Fix a copy-and-paste typo in setup.rst.
Yes, we should remove this action, there are better ones available now. I'll create an issue. |
|
Actually, we have this in #165 already, I renamed it slightly. |
|
Thanks for addressing my comments, I'll merge this now. |
I finally got around to addressing #197 that I opened in June 2022. In doing so, I encountered many basic problems with my local installation (on macOS 13.5.2 with Python 3.11) including running the tests and building the docs. I've tried to fix all the problems that I encountered. Consequently, the scope of this PR is much larger than originally intended. I'll try to summarise the problems and my fixes below.
usage.rstto explain theadd_additional_resourcemethod of theSubmissionandTableobjects. I extended the method to support an additional argumentfile_type="HistFactory"and modified thetest_additional_resource_sizefunction. I modified all five example Jupyter notebooks to use theadd_additional_resourcemethod. Closes Add docs foradd_additional_resourceand supporttypeargument #197.tests.yml.hepdata_lib) written in the__init__method of theSubmissionclass to https://doi.org/10.5281/zenodo.1217998 (the Concept DOI). Closes Change Zenodo record added to Submission to Concept DOI #212..readthedocs.ymlfollowing current recommendations and based on experience with the main HEPData/hepdata repo (closes Update readthedocs config #227). In particular, installinghepdata_libshould (hopefully!) build the module index (closes readthedocs: Module index not available anymore #138). I won't know if it works until I open this PR, and some further tweaks might be needed, so I'm going to open a draft PR to start with.hepdata_lib.c_file_readerandhepdata_lib.helpersmodules to the docs.python setup.py testmentioned indev.rstis deprecated and it does not even work for me ("Ran 0 tests in 0.000s"). It is not used in the CI (tests.yml) which instead usespython -m pytest tests. I've changeddev.rstto instead recommend usingpytest tests. I didn't change another instance ofpython setup.py testinrelease.ymlwhich does not seem to be used recently.tests.yml) has a commandpython -m pip install '.[test]'which givesWARNING: hepdata-lib 0.13.0 does not provide the extra 'test'. This is becauseextras_requireneeds to be defined insetup.py, which I've now done with the requirements needed to run the tests locally on my system. The existing CI still works despite this bug because the test requirements are all installed in the previous step. I've also removed the deprecatedsetup_requiresandtests_require.dev.rst. I addedSphinx<7to thedocs/requirements.txtfile as the build fails with Sphinx v7. I replacedm2r2withsphinx-mdincludeto avoid a version conflict with Mistune when installing Jupyter in the same virtual environment.SPHINXOPTS = -W --keep-goingto the Makefile andfail_on_warning: trueto.readthedocs.yml. This should help avoid building broken docs. For example, the links to the Jupyter notebooks and Binder links in Further examples are not working properly.setup.rstto mention use of the standardvenvmodule rather thanvirtualenv/virtualenvwrapper.wraptv1.12.1 viaastroid. We could upgrade Pylint to a more recent version, but thepylintrcfiles would need to be updated, since they use many obsolete options. For now, I removed support for Python v3.11 and kept Pylint v2.9.6. I added a section on "Analysing the code" indev.rstto get the developer to run the same commands as used in GitHub Actions. I got several error messages that I didn't understand, so for now I simply added# pylint: disable-msgcomments to the offending lines.📚 Documentation preview 📚: https://hepdata-lib--233.org.readthedocs.build/en/233/