Drop support for Python < 3.8 and remove CircleCI#177
Conversation
Co-authored-by: nsaje <156006+nsaje@users.noreply.github.com>
|
Follow-up to #170 (review) . Dropping just the versions that can only be tested on CircleCI for now. Should we remove 3.8 and 3.9 as well since they're EOL? |
This comment was marked as resolved.
This comment was marked as resolved.
Co-authored-by: nsaje <156006+nsaje@users.noreply.github.com>
This comment was marked as resolved.
This comment was marked as resolved.
Co-authored-by: nsaje <156006+nsaje@users.noreply.github.com>
|
This looks good, but if we are actually dropping support for those versions we should also drop checks for
Yes, but it can still wait. By dropping < 3.8 we should be able to drop all the version conditionals already, so it doesn't give us much (and I suspect some users might use |
|
We should also update https://github.com/closeio/ciso8601/blob/master/why_ciso8601.md (Aside: it seems that the Mermaid rendering broke in some way?) |
This comment was marked as resolved.
This comment was marked as resolved.
…1.md Co-authored-by: nsaje <156006+nsaje@users.noreply.github.com>
Co-authored-by: nsaje <156006+nsaje@users.noreply.github.com>
This comment was marked as resolved.
This comment was marked as resolved.
Co-authored-by: nsaje <156006+nsaje@users.noreply.github.com>
This comment was marked as resolved.
This comment was marked as resolved.
Co-authored-by: nsaje <156006+nsaje@users.noreply.github.com>
Co-authored-by: nsaje <156006+nsaje@users.noreply.github.com>
Co-authored-by: nsaje <156006+nsaje@users.noreply.github.com>
Co-authored-by: nsaje <156006+nsaje@users.noreply.github.com>
@movermeyer updated the why_ciso8601.md. Renders normally for me both on main and on this branch 🤔 @thomasst removed outdated C macros |
module.c
Outdated
| ((PY_MAJOR_VERSION == 3 && PY_MINOR_VERSION >= 6) || PY_MAJOR_VERSION > 3) | ||
| #define PY_VERSION_AT_LEAST_37 \ | ||
| ((PY_MAJOR_VERSION == 3 && PY_MINOR_VERSION >= 7) || PY_MAJOR_VERSION > 3) | ||
| #define PY_VERSION_AT_LEAST_38 \ |
module.c
Outdated
| #ifdef PYPY_VERSION | ||
| #define SUPPORTS_37_TIMEZONE_API \ | ||
| (PYPY_VERSION_NUM >= 0x07030600) && PY_VERSION_AT_LEAST_38 | ||
| #define SUPPORTS_37_TIMEZONE_API (PYPY_VERSION_NUM >= 0x07030600) |
There was a problem hiding this comment.
Also possible:
#define SUPPORTS_37_TIMEZONE_API \
(!defined(PYPY_VERSION) || PYPY_VERSION_NUM >= 0x07030600)
My apologies. It works fine. I had reset my JavaScript allow lists, and apparently Mermaid diagram rendering JS is hosted from a different domain than the rest of GitHub 🙈 |
| A--yes-->Y; | ||
| A--no-->C; | ||
|
|
||
| C[Do you need to support Python 2.7?]; |
There was a problem hiding this comment.
Another option to consider is to point to ciso8601 v2.x (with the assumption that we'll do a v3.x release for these changes).
- Remove unused PY_VERSION_AT_LEAST_38 macro (thomasst) - Simplify SUPPORTS_37_TIMEZONE_API macro using suggested syntax (thomasst) - Add section for Python < 3.8 support pointing to v2.x (movermeyer) - Update flowchart to include v2.x option for older Python versions Co-authored-by: nsaje <156006+nsaje@users.noreply.github.com>
What are you trying to accomplish?
Addresses three unresolved review comments from thomasst and movermeyer on PR #177:
PY_VERSION_AT_LEAST_38macroSUPPORTS_37_TIMEZONE_APImacro definitionWhat approach did you choose and why?
C code cleanup (module.c)
PY_VERSION_AT_LEAST_38macro - confirmed unused via grep across codebase#ifdef PYPY_VERSIONblock with single-line macro using thomasst's suggested syntax:Documentation (why_ciso8601.md)
What should reviewers focus on?
SUPPORTS_37_TIMEZONE_APImacro logic is equivalent to the originalThe impact of these changes
No functional changes. Improves code maintainability and provides clear upgrade/downgrade path for users.
Testing
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.