gouline / dbt-metabase

dbt + Metabase integration
https://pypi.org/project/dbt-metabase/
MIT License
442 stars 63 forks source link

Minimum Python required version is not correct #52

Closed fernandobrito closed 2 years ago

fernandobrito commented 2 years ago

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:

Orthogonal to that, we could:

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

gouline commented 2 years ago

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

z3z1ma commented 2 years ago

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