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

Minimum Python required version is not correct #52

Closed
fernandobrito opened this issue Aug 30, 2021 · 2 comments · Fixed by #55
Closed

Minimum Python required version is not correct #52

fernandobrito opened this issue Aug 30, 2021 · 2 comments · Fixed by #55
Assignees
Labels
bug Something isn't working

Comments

@fernandobrito
Copy link
Contributor

fernandobrito commented Aug 30, 2021

Hi!

The docs state that this amazing library "Requires Python 3.6 or above", and also setup.py states the same.

However, some recently introduced changes (https://github.com/gouline/dbt-metabase/blob/master/dbtmetabase/models/metabase.py#L28) broke that. Using typing.Literal requires Python 3.8+. Running the library with Python 3.7 for example will raise:

ImportError: cannot import name 'Literal' from 'typing' (/Library/Frameworks/Python.framework/Versions/3.7/lib/python3.7/typing.py)

Alternatives on what to do:

  • Remove this Literal type hint and instead use Enum
  • Install an extra dependency that will make this type hint work in 3.6+ (see info box on https://mypy.readthedocs.io/en/stable/literal_types.html)
  • Bump the minimum required version to 3.8. Maybe a bit overkill just because of a type hint. dbt itself doesn't run well on 3.9, so requiring a minimum of 3.8 leaves a very tight compatibility range.

Orthogonal to that, we could:

  • Explicitly set a Python version (3.6) in the GitHub Actions workflow that runs CI tests using:
- uses: actions/setup-python@v2
  with:
    python-version: '3.6.x' 

I'm happy to contribute with a PR once the maintainer and contributors align on how to move forward 😄 .

@fernandobrito fernandobrito changed the title Minimum required version is not correct Minimum Python required version is not correct Aug 30, 2021
@gouline
Copy link
Owner

gouline commented Aug 31, 2021

Hmm, interesting!

I think removing Literal is fine, since it's just a type annotation and doesn't change any functionality. Definitely wouldn't want to make 3.8 the minimum Python version or introduce an extra dependency just for that.

Also good idea on running the tests in GitHub Actions. Let's start with 3.6 for now and consider the version matrix as a future improvement.

Let me know if you have any other questions to raise a PR for this.

FYI @z3z1ma

@gouline gouline added the bug Something isn't working label Aug 31, 2021
@gouline gouline added this to the v0.8.x milestone Aug 31, 2021
@z3z1ma
Copy link
Contributor

z3z1ma commented Aug 31, 2021

Thanks for pointing that out! Agreed. I like the Enum class actually. Its still nice and explicit and backwards compatible as fernandobrito pointed out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants