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(scripts): Type Hinting wit Union type instead of | operator #1780

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

poting-lin
Copy link
Contributor

It returns the below message when I run it locally:
TypeError: unsupported operand type(s) for |: 'types.GenericAlias' and 'NoneType'

After checking serval possibilities, it would be nice if we could refactor part of type hints.

thanks!

@manast
Copy link
Contributor

manast commented Apr 4, 2023

Could you please explain what is the difference between both approaches?

@nullndr
Copy link
Contributor

nullndr commented Apr 4, 2023

This shouldn't be necessary, see the docs the typings.Tuple type. They explicity say that X | Y is recommended.

@poting-lin
Copy link
Contributor Author

I'm using Python 3.9.16, which doesn't support the shorthand (X | Y).
It would be nice if we could support earlier versions of Python by refactoring the code with Union.

Or the project only supports the Python version above 3.10?

Thanks!

@manast
Copy link
Contributor

manast commented Apr 5, 2023

I have not been a python user for many years so I am not so familiar on which versions should be supported nowadays.

@poting-lin
Copy link
Contributor Author

I would say supporting 3.7 above would be great, but it depends on the capacity of contributors, it needs a lot of testing for different versions.
there is the version support for some of packages on pip:

  • Celery:
    Requires: Python >=3.7
  • Flask
    Python >=3.7
  • Fastapi
    Python >=3.7
  • Redis
    Python >=3.7
  • Pandas
    Python >=3.8

@poting-lin
Copy link
Contributor Author

@nullndr the project support 3.10 above only?

@nullndr
Copy link
Contributor

nullndr commented Apr 12, 2023

@poting-lin I'm the the one that should choose such constraints, you should ask @manast.

However, the latest version of Python is 3.11.3 so I think it should probably better to keep with the latest major releases (11 and 10)

@poting-lin
Copy link
Contributor Author

@nullndr Thanks for the reply, the reason why I ask is because if we could allow other projects under Python 3.10 to implement bullMQ by refactoring a tiny part of the code, it would bring more compatibility and allow more projects to implement bullMQ.

like you said, maybe I will wait until @manast answer the question.

@manast
Copy link
Contributor

manast commented Apr 12, 2023

I think a good approach could be to follow what the Redis module supports because that would be the least common denominator anyway since we depend on Redis, and if you are already using the Redis ecosystem you most likely also depend on the Redis module.

What do you think?

@poting-lin
Copy link
Contributor Author

@manast good idea, bullMQ applying redis 4.5.4, and based on the document on pip, it is Python >=3.7

means bullMQ support Python >=3.7?

@manast
Copy link
Contributor

manast commented Apr 13, 2023

@manast good idea, bullMQ applying redis 4.5.4, and based on the document on pip, it is Python >=3.7

means bullMQ support Python >=3.7?

Yeah, actually I think so. Even though I am all for supporting the new features in more modern versions, I also recognize that most users are not using those versions yet. I think that having Redis's python requirements as a baseline is a good tradeoff. Is there any good way to enforce a given version in a GitHub action so that we do not use unsupported features by mistake?

@poting-lin
Copy link
Contributor Author

sorry for the late reply, to be able to test the code with multiple versions, we may implement tox
https://github.com/poting-lin/tox-example

I wrote the functions with the examples: with/without shorthand (X | Y) syntax to test it.

it shows the issues in each file for each Python version and shows the most compatible Python versions at the end of the report.

image

@manast @nullndr do you think it is an idea to implement in the pipeline?

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.

None yet

3 participants