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

Restore Python 3.6+ compatibility #55

Merged
merged 10 commits into from
Sep 13, 2021

Conversation

fernandobrito
Copy link
Contributor

@fernandobrito fernandobrito commented Sep 4, 2021

Resolves #52

  • Reverts using Literal so the project runs on Python 3.6+
  • Uses Python 3.6.x on the GitHub Actions to make sure the project runs on Python 3.6+

@fernandobrito
Copy link
Contributor Author

I need some help with this one. I can't build the project on Python 3.6.

image

It seems the setuptools bundled with this version is too old for setuptools_scm, and you can't add setuptools as a setup_requires. One of the suggestions from the error message is to explicitly install the setup tools (like I am doing in this PR in the GitHub Action to test it out), but what are your thoughts on that?

@fernandobrito
Copy link
Contributor Author

Another place where I need feedback: the package version during runtime magic that you use from https://pypi.org/project/setuptools-scm/#retrieving-package-version-at-runtime requires importlib.metadata which is only available on Python 3.8+. The page points to a library for backward compatibility. Do we want to use it?

@gouline gouline marked this pull request as draft September 5, 2021 23:51
@gouline
Copy link
Owner

gouline commented Sep 5, 2021

I converted this to draft for you, click "Ready for review" when it's no longer WIP.

@gouline gouline self-requested a review September 6, 2021 02:33
@gouline gouline added this to the v0.8.x milestone Sep 6, 2021
@fernandobrito fernandobrito marked this pull request as ready for review September 10, 2021 15:04
@fernandobrito
Copy link
Contributor Author

fernandobrito commented Sep 10, 2021

Thanks! Ready to be reviewed now.

  • For the use of Literal, I've replaced it with an Enum
  • For the issue with setuptools_scm from my screenshot above, I took the suggestion from the error message and installed an appropriate version of setuptools on GitHub Actions
  • For the issue onimportlib not being available before Python 3.8, I wrapped it on a try block, which worked well locally.

But I'm more than open to suggestions on different approaches to solve those 3 issues.

Also, is it fine if I "Squash and merge" once it's time to merge? The commit history in the branch is a bit messy 😄

@fernandobrito fernandobrito changed the title Set explicit Python version on GitHub Actions Restore Python 3.6+ compatibility Sep 10, 2021
Copy link
Owner

@gouline gouline left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a couple of initial comments.

@z3z1ma should also review, since most of it is his code.

.github/workflows/master.yml Outdated Show resolved Hide resolved
.github/workflows/pull_request.yml Outdated Show resolved Hide resolved
dbtmetabase/utils.py Outdated Show resolved Hide resolved
z3z1ma
z3z1ma previously approved these changes Sep 11, 2021
Copy link
Contributor

@z3z1ma z3z1ma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything looks accounted for on the Enum update throughout the package. Great job & thanks.

Copy link
Owner

@gouline gouline left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for your efforts 🚀

@gouline gouline merged commit 696d417 into gouline:master Sep 13, 2021
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.

Minimum Python required version is not correct
3 participants