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

feat: drop Python 3.7 support #417

Merged
merged 2 commits into from
Nov 30, 2023
Merged

feat: drop Python 3.7 support #417

merged 2 commits into from
Nov 30, 2023

Conversation

tombh
Copy link
Collaborator

@tombh tombh commented Nov 26, 2023

@karthiknadig I think it's been 3 months since Python 3.7's EOL?

pyproject.toml Outdated
@@ -13,7 +13,7 @@ license = "Apache 2.0"
readme = "README.md"

[tool.poetry.dependencies]
python = ">=3.7.9,<4"
python = ">=3.8.18,<4"
Copy link
Contributor

@dimbleby dimbleby Nov 26, 2023

Choose a reason for hiding this comment

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

specifying a patch version at the lower bound is unusual - why not >=3.8?

(the upper bound is also controversial, at least in some circles. Since there is no imminent prospect of python 4 it probably does not matter, but just python = ">=3.8" as the entire constraint should be fine).

I also notice that there are quite a few outdated dev dependencies here. My preference for dev dependencies is >= constraints rather than caret or specific versions, your mileage may vary

Copy link
Contributor

Choose a reason for hiding this comment

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

also you forgot to update poetry.lock

@tombh tombh force-pushed the tombh/drop-python-3.7 branch from 276d35e to 2aa1c0f Compare November 26, 2023 17:08
@tombh
Copy link
Collaborator Author

tombh commented Nov 26, 2023

Great, thanks for that. How's that now? I also add a new lint sub-task to check if poetry.lock is up to date.

Still need to disable Python 3.7 in the Github Action matrix.

@tombh tombh force-pushed the tombh/drop-python-3.7 branch 2 times, most recently from 7fe3809 to 09265f7 Compare November 26, 2023 17:15
@dimbleby
Copy link
Contributor

looks good to me, though I think libraries should be cautious of "poetry up". By bumping lower bounds as high as they will go, it increases the chances of unsolvable environments. (Curiously, tight upper bounds causing the same thing seem to get a lot more attention: but an upper bound can only be incompatible with a corresponding lower bound!)

eg you have bumped websockets to ">=12": but if older versions continue to work fine for pygls then there is a case for preferring ">=11.0.3"

policy = asyncio.get_event_loop_policy()
# @alcarney is this needed if we drop 3.7 suppoer?
# if sys.version_info.minor == 7 and IS_WIN:
# policy = asyncio.WindowsProactorEventLoopPolicy()
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be safe to delete now

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks

@tombh tombh force-pushed the tombh/drop-python-3.7 branch from 09265f7 to 840f7b4 Compare November 30, 2023 15:02
@tombh
Copy link
Collaborator Author

tombh commented Nov 30, 2023

Thanks David, that's another really good point. What would you say is a good rule of thumb for updating dependencies? Say, for example: beyond setting a minimum version that supports required functionality, we aim to be one major version below the latest. Obviously that's not a strict rule, like for cattrs that doesn't follow semver, it's just a rule of thumb.

@tombh tombh force-pushed the tombh/drop-python-3.7 branch from 840f7b4 to 7aeca69 Compare November 30, 2023 15:06
@dimbleby
Copy link
Contributor

I don't know a good rule except to use your best judgement.

I reckon that approximately no-one runs tests to verify that their code works with their declared lower bounds. So it can be pretty easy to inadvertently start using features that were new in eg websockets 12 without realising that the claimed "webockets >= 11" has become a lie.

So you are navigating between the rock and the hard place: make some sort of best effort not to have unreasonably narrow requirements, but also don't leave those requirements so broad that they're probably false.

I doubt that it's worth more than some modest amount of care though: users will tell you about it when you get it wrong!

Websockets isn't at latest to help with downstream dependency resolution
@tombh tombh force-pushed the tombh/drop-python-3.7 branch from 7e1d376 to 660d1c2 Compare November 30, 2023 20:53
@tombh
Copy link
Collaborator Author

tombh commented Nov 30, 2023

That's fair, so at least be aware of the tradeoffs and try to find a balance without sweating too much over it.

Thanks for all your advice on this, I really appreciate it.

@tombh tombh requested a review from dimbleby November 30, 2023 20:59
@tombh tombh merged commit 53093c1 into main Nov 30, 2023
16 checks passed
@tombh tombh deleted the tombh/drop-python-3.7 branch November 30, 2023 21:05
@tombh tombh mentioned this pull request Jan 28, 2024
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.

3 participants