[JOSS] Initial comments about code and repository

[krachyon]

Basing this on commit 011333f84486614cb9339d3874dc072c45ebed23, using Python 3.10.5 (main, Jun 6 2022, 18:49:26) [GCC 12.1.0] on linux

• A few things are missing from your readme:

  • A Contributing section, with expected code style and possible adaptations of pep8 and ideally a description how the software came to be and how/if you plan to develop it further (e.g. "I plan on adding feature X", "I would be really glad if someone could help with Y")
  • How you're supposed to run the tests (and ideally linters)

• Data originally checked into and subsequently removed is still visible/available in the history, so every clone of the repository will still download the files. If you didn't plan on publishing it, you should re-write the repository history with something like an interactive rebase.

• There are a bunch of issues reported by pylint, flake8 and check-manifest. I'd suggest running something like autopep8 or black on the code, that should fix the majority of issues. If you really want to deviate from the pep8 style, you should include the relevant linter config files as part of the repository (they seem to be manually selected in the gitub-ci config?), so the checks can be run locally. A tool like tox would be a helpful addition so that one can check their work with a single command before they commit/push.

  • running the tests with python -m unittest tests/* creates a load of warnings relating to the fits-file handling. It fails with the following message

    ERROR: tests/__pycache__ (unittest.loader._FailedTest)
    ----------------------------------------------------------------------
    ImportError: Failed to import test module: tests/__pycache__
    Traceback (most recent call last):
    File "/usr/lib/python3.10/unittest/loader.py", line 154, in loadTestsFromName
    module = __import__(module_name)
    ModuleNotFoundError: No module named 'tests/__pycache__'

• when running tests with pytest (as I think it was intended?), some warnings remain. I suggest silencing the warnings you expect to happen so a new warning introduced by future additions will actually be spotted.

[temuller]

I am having issues trying to understand the second points:

Data originally checked into and subsequently removed is still visible/available in the history, so every clone of the repository will still download the files. If you didn't plan on publishing it, you should re-write the repository history with something like an interactive rebase.

Are you talking about files that are no longer needed but are saved in the history? Could you please rephrase this?

[krachyon]

Are you talking about files that are no longer needed but are saved in the history? Could you please rephrase this?

Yes, when looking at the git-history you can still see (and access) e.g. images/SN2006bh/PS1_g.fits, the file's contents are still automatically downloaded when the repository is cloned. If you happen to change them slightly, all versions that once existed have to be transferred on clone, this can blow up your repository size very quickly and is why typically you should not add binaries to version control.

If those files are something you did not want to publish, you'll have to delete them from the history. Some context and a solution to do that are here https://stackoverflow.com/questions/58161926/permanently-removing-a-file-from-git-history

[krachyon]

In a recent commit you commented out the __pycache__ entry in .gitignore, these dirs should not be added to version control. They contain machine-specific information like &/home/tomas/hostphot/tests/__init__.py which can cause pretty confusing errors for others.

[temuller]

Thanks, I will remove this as well.

[krachyon]

I'll mark this as resolved with the review done...