Closed GraemeWatt closed 9 months ago
Merging #233 (0d77f11) into main (9330eb9) will increase coverage by
0.02%
. The diff coverage is100.00%
.
:exclamation: 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
Flag | Coverage Δ | |
---|---|---|
unittests-3.10 | 88.54% <100.00%> (+0.02%) |
:arrow_up: |
unittests-3.6 | 88.21% <100.00%> (+0.02%) |
:arrow_up: |
unittests-3.7 | 88.21% <100.00%> (+0.02%) |
:arrow_up: |
unittests-3.8 | 88.34% <100.00%> (+0.02%) |
:arrow_up: |
unittests-3.9 | 88.34% <100.00%> (+0.02%) |
:arrow_up: |
Flags with carried forward coverage won't be shown. Click here to find out more.
Files | Coverage Δ | |
---|---|---|
hepdata_lib/__init__.py | 90.57% <100.00%> (+0.05%) |
:arrow_up: |
hepdata_lib/helpers.py | 77.69% <ø> (ø) |
@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!
@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.
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?
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 .rst
files when running Sphinx. I've now removed the # pylint: disable-msg
comments that I added previously and fixed the Pylint errors by making some other changes (adding __init__.py
, importing test_utilities.py
as a relative module after hepdata_lib
, adding math
and array
modules to extension-pkg-whitelist
in pylintrc
). My setup is not unusual (macOS 13.5.2 with Python 3.10), so I don't know why these errors didn't show up previously in the CI.
I also renamed the master
branch to main
as for other HEPData repositories (hope you don't mind). I updated the actions in the GitHub Actions workflows to hopefully close some of the open Dependabot PRs.
Is the release.yml
workflow currently being used? If not, removing it from the repository would reduce maintenance. It seems to use many unofficial actions. I didn't update the python setup.py test
line. If there's a reason why actions should not be upgraded, the dependabot.yml
file should have an ignore
option.
Is the
release.yml
workflow currently being used? If not, removing it from the repository would reduce maintenance. It seems to use many unofficial actions. I didn't update thepython setup.py
test line. If there's a reason why actions should not be upgraded, the dependabot.yml file should have anignore
option.
Yes, we should remove this action, there are better ones available now. I'll create an issue.
Actually, we have this in https://github.com/HEPData/hepdata_lib/issues/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.rst
to explain theadd_additional_resource
method of theSubmission
andTable
objects. I extended the method to support an additional argumentfile_type="HistFactory"
and modified thetest_additional_resource_size
function. I modified all five example Jupyter notebooks to use theadd_additional_resource
method. Closes #197.tests.yml
.hepdata_lib
) written in the__init__
method of theSubmission
class to https://doi.org/10.5281/zenodo.1217998 (the Concept DOI). Closes #212..readthedocs.yml
following current recommendations and based on experience with the main HEPData/hepdata repo (closes #227). In particular, installinghepdata_lib
should (hopefully!) build the module index (closes #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_reader
andhepdata_lib.helpers
modules to the docs.python setup.py test
mentioned indev.rst
is 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.rst
to instead recommend usingpytest tests
. I didn't change another instance ofpython setup.py test
inrelease.yml
which 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_require
needs 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_requires
andtests_require
.dev.rst
. I addedSphinx<7
to thedocs/requirements.txt
file as the build fails with Sphinx v7. I replacedm2r2
withsphinx-mdinclude
to avoid a version conflict with Mistune when installing Jupyter in the same virtual environment.SPHINXOPTS = -W --keep-going
to the Makefile andfail_on_warning: true
to.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.rst
to mention use of the standardvenv
module rather thanvirtualenv
/virtualenvwrapper
.wrapt
v1.12.1 viaastroid
. We could upgrade Pylint to a more recent version, but thepylintrc
files 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.rst
to 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-msg
comments to the offending lines.:books: Documentation preview :books:: https://hepdata-lib--233.org.readthedocs.build/en/233/