JOSS Review: Python-related code issues

[SimonMolinsky]

💡 Description

The code within the package is inconsistent with Python 3.X, and there are several medium and weak issues. Here's the list of things to correct,

• init method should return None (object is returned),
• A class inheritance from the object - not required in Python 3.X.
• Definition of instance attributes OUTSIDE the init method, it is tough to track all class attributes in this case, and code is susceptible to changes - define attributes in the __init__ as a None,
• Some variables are not used,
• Multiple cases where built-in names are shadowed: list, format, type, bytes, dir, id,
• Class __init__ method parameters type is defined as object, but it can be defined strictly as a specific Class.

URL to JOSS review: openjournals/joss-reviews#4327

[SimonMolinsky]

Definition of instance attributes OUTSIDE the init method, it is tough to track all class attributes in this case, and code is susceptible to changes - define attributes in the __init__ as a None

• the task is related to the exceptions that are an important part of a process, it requires a lot of refactoring, so for now it is halted.

[SimonMolinsky]

Hi @marcsit ,

I've made a PR into the package repo here: #18

If you need more info, just let me know; the package is accepted from my side, and now it's only a matter of improvement. As you maybe see in the comment above, there is a problem with class attributes (those with self.) defined outside the __init__ method. Your program catches exceptions where those attributes are undefined, and this is fine because it works, but it could be improved to be more "Pythonic." This issue is related to another problem - with broad exceptions. Those are very dangerous; we can catch something we don't want to, and we won't know about it; this is an antipattern in Python.

There are a few more issues, but too much for one thread is not a good idea :)

So just in case: will you be a maintainer in the future? Will there be anybody to accept Issues / check PRs?

[marcsit]

@SimonMolinsky I am going to be the main maintainer for a while although is not part of my job anymore. There should be a person from NAIF/JPL, the job announcement is now posted (my replacement.) And they should become the main maintainer. For the time being I will assign this issue to myself for the class attributes issues.

[marcsit]

In order to support finding these issues, include the following lines in .pre-commit-config.yaml

They were removed in order to bypass the mypy errors, but these are useful in the context of this issue:

• repo: local
  hooks:
  • id: mypy
    name: mypy
    entry: mypy src
    language: system
    pass_filenames: false

[SimonMolinsky]

@marcsit I've asked because I think I can do something good here, but I'd rather wait for the publication in JOSS to avoid any potential delays. I hope that your work will be published soon! After publication, I'll "activate myself" here and hopefully, we will be able to delegate some small fixes for me (?)

[marcsit]

@SimonMolinsky absolutely! Thanks for that.