Set default python version 3.9 and fix strenum problem #135
Conversation
jacomago
requested review from
anderslindho,
ralphlange,
shroffk,
simon-ess and
tynanford
|
I tried to get this check in the pre-commit, but adding all ruff lints wasn't enough, I had to add type checking... Which made the PR huge. So I'll stick with just this change for now. |
|
|
||
| [tool.ruff] | ||
| line-length = 120 | ||
| target-version = "py39" |
There was a problem hiding this comment.
This doesn't mesh well with your attempt to externalise python version into a variable
There was a problem hiding this comment.
Not sure I can fix that.
There was a problem hiding this comment.
You can abandon the idea to externalise it :)
There was a problem hiding this comment.
You mean out of the dockerfile?
server/pyproject.toml
Outdated
|
|
||
| [tool.pytest.ini_options] | ||
| log_cli = true | ||
| log_cli_level = "DEBUG" |
There was a problem hiding this comment.
Do we really need this as default?
There was a problem hiding this comment.
Its used to make the logs published by docker appear in the ci logs.
There was a problem hiding this comment.
Isn't this setting global, i.e. would apply also if I clone and run locally?
Can't it be specified on the command-line when executing pytest?
There was a problem hiding this comment.
Yeah, you can override it. But sure tests running with debug enabled makes sense?
There was a problem hiding this comment.
IMO no, since pytest is configured by default to display log messages over a certain verbosity only for failing tests, and since it is an easy modification for any user to alter the verbosity if necessary.
There was a problem hiding this comment.
I'm with @jacomago on this one. If the tests are failing for some reason in CI but are not reproducible locally, this is pretty useful.
There was a problem hiding this comment.
My point is not to remove it from CI, but that this is a global config. We can still lower the verbosity for CI.
There was a problem hiding this comment.
Just change https://github.com/ChannelFinder/recsync/blob/master/.github/workflows/server.yml#L69 to pytest --log-cli-level=DEBUG
Having the matrix wasn't making that much difference since recceiver is running in a container anyway
jacomago
force-pushed
the
set-default-python-version-lower
branch
from
7dd8ea3 to
e3862b6
Compare
|
| strategy: | ||
| matrix: | ||
| include: | ||
| - python-version: "3.6" |
There was a problem hiding this comment.
should remove 3.6 - 3.8 in the CI
| import logging | ||
| import random | ||
|
|
||
| from zope.interface import implementer |
There was a problem hiding this comment.
is moving these imports necessary? same goes for all the other import changes
| description="resync server" | ||
| version="1.5" | ||
| readme = "README.md" | ||
| requires-python = ">=3.6" |
| version="1.5" | ||
| readme = "README.md" | ||
| requires-python = ">=3.6" | ||
| classifiers = [ |
| "Programming Language :: Python :: 3.13", | ||
| "Programming Language :: Python :: 3.14", | ||
| ] | ||
| dependencies = [ |
There was a problem hiding this comment.
with >= 3.9 , we could use StrEnum and use this pip package like you mentioned? https://pypi.org/project/StrEnum/
4 participants
No description provided.