-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
JSON string 'format' kwarg #984
Conversation
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.
Additional details and impacted files@@ Coverage Diff @@
## main #984 +/- ##
==========================================
- Coverage 70.10% 61.43% -8.67%
==========================================
Files 62 62
Lines 4449 4468 +19
==========================================
- Hits 3119 2745 -374
- Misses 1330 1723 +393 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would like to see some tests for each 'format'
"time": r'\d{2}:\d{2}:\d{2}(?:\.\d+)?(?:Z|[+-]\d{2}:\d{2})?', | ||
"date": r'\d{4}-\d{2}-\d{2}', | ||
"duration": r'P(\d+Y)?(\d+M)?(\d+D)?(T(\d+H)?(\d+M)?(\d+S)?)?', | ||
# Email addresses |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are probably not comprehensive (per RFC 5322), but are a good starting point :-)
@Harsha-Nori @riedgar-ms feeling pretty good about this. Some notes:
|
Could alternatively |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer xfail to skip. That way if they do start passing we get to find out about it (albeit through a test run failure, until the xfail is removed). And there's no real time constraint, compared to the tests which run against an actual LLM.
"bad_str", | ||
[ | ||
'"2020-01-32"', # a invalid date string with 32 days in January | ||
pytest.param('"2021-02-29"', marks=pytest.mark.xfail(reason="number of days not yet tied to month")), # a invalid date string with 29 days in February (normal) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want some extra challenge, remember that 2000 was a leap year, but that 1900 wasn't ;-)
'"008:030:006Z"', # invalid time string with extra leading zeros | ||
'"8:3:6Z"', # invalid time string with no leading zero for single digit | ||
'"8:0030:6Z"', # hour, minute, second must be two digits | ||
pytest.param('"22:59:60Z"', marks=pytest.mark.xfail(reason="leap seconds are hard")), # invalid leap second, Zulu (wrong hour) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy to see leap seconds acknowledged :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't take credit 😉
pytest.param('"\\"joe@bloggs\\"@example.com"', marks=pytest.mark.xfail(reason="Quoted strings not yet implemented in local part")), # a quoted string with a @ in the local part is valid | ||
'"joe.bloggs@[127.0.0.1]"', # an IPv4-address-literal after the @ is valid | ||
pytest.param('"joe.bloggs@[IPv6:::1]"', marks=pytest.mark.xfail(reason="IPv6 is hard")), # an IPv6-address-literal after the @ is valid | ||
'"[email protected]"', # two separated dots inside local part are valid |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small extra one (since things not supporting it bug me): can you include a local part with a +
in the middle, like GMail supports?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can do :)
Implement (some)
format
strings for JSON according to standards and RFCs listed here