FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Require a decimal point for fractional seconds by SashaMIT · Pull Request #1280 · PyMySQL/PyMySQL · GitHub

  1. PyMySQL
  2. PyMySQL

Repository navigation

Require a decimal point for fractional seconds - #1280

Merged
methane merged 1 commit into
PyMySQL:mainfrom
SashaMIT:fix/fraction-separator
Oct 2, 2026
ConversationCommits (1)ChecksFiles changed
Merged

Require a decimal point for fractional seconds#1280
methane merged 1 commit into
PyMySQL:mainfrom
SashaMIT:fix/fraction-separator

Conversation

SashaMIT commented Oct 1, 2026 •
edited by coderabbitai Bot
Loading

Copy link
Copy Markdown
Contributor

Summary

  • convert_time("23:06:20X511581") returned time(23, 6, 20, 511581). The fractional pattern used an unescaped dot, so X was read as the separator and 511581 as microseconds.
  • convert_datetime("2007-02-24 23:06:20X511581") and convert_timedelta("25:06:17X511581") did the same. The match also did not have to consume the rest of the value, so escaping the dot alone still returned a time of the prefix.
  • A real fraction is unchanged. 23:06:20.511581 is still time(23, 6, 20, 511581). 2007-02-24 23:06:20 is still that datetime. -25:06:17.1 is still the same timedelta. The doctests say an illegal value comes back as the original string.

Test plan

  • Red before the change: the three new tests failed. Datetime, time, and timedelta each returned a value with 511581 microseconds.
  • Green after: python -m unittest pymysql.tests.test_converters (12 tests, ok). doctest.testmod(pymysql.converters) was 13 passed.

Summary by CodeRabbit

  • Bug Fixes
    • Improved date and time conversion accuracy: fractional seconds must use a period, and values with unmatched extra text are no longer incorrectly converted.

Require a decimal point for fractional seconds
5d13ee5 
The datetime, time, and timedelta converters treated any character as
the fractional separator and did not require the rest of the value to
match. 23:06:20X511581 came back as a time with 511581 microseconds.
An illegal value is supposed to come back as the original string.

coderabbitai Bot commented Oct 1, 2026 •
edited
Loading

Copy link
Copy Markdown

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info ⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 19ef96c5-3e3a-45db-b21f-fc42da1432ea

📥 Commits

Reviewing files that changed from the base of the PR and between bb7059e and 5d13ee5.

📒 Files selected for processing (2)
  • pymysql/converters.py
  • pymysql/tests/test_converters.py

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review.

📝 Walkthrough

Walkthrough

The datetime, timedelta, and time patterns now require a literal period before fractional seconds. Their converters require the full input to match, with existing fallback behavior retained. Tests cover inputs that use X as the separator.

Changes

Datetime and time conversion

Layer / File(s) Summary
Strict matching, conversion, and tests
pymysql/converters.py, pymysql/tests/test_converters.py		 The datetime, timedelta, and time patterns require a literal period before fractional seconds. Their converters require a full match. Tests check that inputs with X as the separator are not converted.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~8 minutes

Change: Bug fix

Suggested reviewers: methane

Merge Risk: ⚪ Minimal · up to 5d13e

Malformed separators and partial temporal strings remain unconverted, while valid fractional values retain coverage; no actionable merge-blocking risk is evident.

Security Architecture Review

Security architecture risk: ⚪ Minimal · up to 5d13e

The change tightens temporal parsing without adding privileges, dependencies, or new data access. Malformed values may now remain strings rather than become temporal objects, consistent with the existing invalid-input behavior. No material security risk was identified in the affected conversion path.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — Inputs controlled by a database server or direct converter caller can affect returned value representation through the existing parsing path. The PR narrows accepted temporal syntax without expanding reachability or authority. Application-specific handling of fallback strings remains outside the inspected scope.

Trust Boundaries and Controls

  • observed — The changed controls validate temporal syntax, not identity or authorization. Result values traverse the same decoder path as before, and mismatches use existing fallback behavior rather than introducing a new privileged operation or trust transition.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 12 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: requiring a decimal point as the fractional-seconds separator.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 🧪 Generate unit tests (beta)
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

codecov Bot commented Oct 2, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.15%. Comparing base (1e28be8) to head (5d13ee5).
⚠️ Report is 81 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1280      +/-   ##
==========================================
- Coverage   86.24%   85.15%   -1.10%     
==========================================
  Files          17       17              
  Lines        2436     2486      +50     
  Branches      258      253       -5     
==========================================
+ Hits         2101     2117      +16     
- Misses        249      301      +52     
+ Partials       86       68      -18

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

methane merged commit 31126a3 into PyMySQL:main Oct 2, 2026
14 checks passed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

Back | FazBrowse Home | New Git URL