Skip to content
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

Fix type union for compatability with python <3.10 #30

Closed
wants to merge 1 commit into from

Conversation

Jean1995
Copy link
Collaborator

@Jean1995 Jean1995 commented Mar 7, 2023

Execution of panama on Python 3.8 leads to the error

$ panama
Traceback (most recent call last):
  File "/xxx/panama", line 5, in <module>
    from panama.cli import cli
  File "xxx/panama/cli.py", line 6, in <module>
    from .run import run
  File "/xxx/panama/run.py", line 92, in <module>
    primary: int | dict,
TypeError: unsupported operand type(s) for |: 'type' and 'type'

This should be the same issue as here

This is one possible fix for this to ensure compatibility with python versions older than 3.10

@The-Ludwig
Copy link
Owner

Thanks for the addition. I guess CI fails since you don't have the CORSIKA download password set in your clone of the repository. I invited you to this repo, so you can simply create a branch next time and the tests will run.

Sadly, this won't fix the error since a quick grep -r "|" ./panama reveals, that I also use | in panama/read.py.

I think this package should use modern python. So I think it's better to do something like from __future__ import annotations to get PEP 604 support (unsure if this really does the trick, needs testing). Or just to require python 3.10, which I realize is not the default for most people using Ubuntu's python or the default anaconda python.

Also I should add matrix verification to automatically test for different python versions and macOS/windows also. The tests are very costly though, since CORSIKA7 has to be compiled every time. I have limited runtime on github... When this project is more stable and mature, maybe we can move it to TUDO-APP...

@Jean1995
Copy link
Collaborator Author

Jean1995 commented Mar 8, 2023

I think this package should use modern python. So I think it's better to do something like from __future__ import annotations to get PEP 604 support (unsure if this really does the trick, needs testing). Or just to require python 3.10, which I realize is not the default for most people using Ubuntu's python or the default anaconda python.

I'd rather do something that is compatible with "older" python versions, since I believe many people interested in this package might be stuck with older python versions.

Also I should add matrix verification to automatically test for different python versions and macOS/windows also. The tests are very costly though, since CORSIKA7 has to be compiled every time. I have limited runtime on github... When this project is more stable and mature, maybe we can move it to TUDO-APP...

Not sure what the "requirements" are to move the project to tudo-app, but in the long-term, I would second that idea

@The-Ludwig
Copy link
Owner

The-Ludwig commented Mar 8, 2023

I'd rather do something that is compatible with "older" python versions, since I believe many people interested in this package might be stuck with older python versions.

I agree, from __future__ import annotations does exactly this. Which I added in this #32 separate PR, so the CI actually works.

Edit: BTW, I have a big, upcoming PR #27, which will add a lot more type annotations to make this repository compatible with the sci-kit hep standards. Need to check with them, maybe they also want to adapt this to the sciki-hep organization, which would definitely be beneficial for visibility, but would need a few more maintainers...

@The-Ludwig The-Ludwig closed this Mar 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants