perf: cast dates with the format inferred by csv-detective - #470
perf: cast dates with the format inferred by csv-detective#470ThibaudDauce wants to merge 6 commits into
Conversation
| "asyncpg>=0.29.0", | ||
| "coloredlogs>=15.0.1", | ||
| "csv-detective==0.12.0", | ||
| "csv-detective", |
There was a problem hiding this comment.
I would not unpin the csv detective version in pyproject, since it's a base component of hydra we need to track and pin the version explicitly by bumping it manually. The manual version change in the pyproject file is an established process in the repo and a good way to protect from human errors.
There was a problem hiding this comment.
I think it can be reverted as soon as the new version is released (this hydra branch require the new csv-detective of datagouv/csv-detective#245)
There was a problem hiding this comment.
I would not merge this branch before there is a version of csv-detective released for that!
| [tool.uv] | ||
| constraint-dependencies = ["urllib3>=2.7.0"] | ||
|
|
||
| # `cast()` only takes the inferred date format from that branch on. Pin it back to a released |
There was a problem hiding this comment.
I would prefer not to add that kind of shim to the version control which decreases readability, maintainability and not as explicit ad the version in pyproject.
Better to create a release of csv-detective and pin the version of the module in pyproject so that it's very explicit.
There was a problem hiding this comment.
I don't understand, this PR require the new version of csv-detective and cannot be merge without it… This comment is just here to remind us to change this, no?
There was a problem hiding this comment.
I would not merge this branch before there is a version of csv-detective released for that!
| @pytest.mark.parametrize( | ||
| "values_expected", | ||
| ( | ||
| # a value no other value of the column disambiguates is read day-first |
There was a problem hiding this comment.
What about the date format YYYY/MM/DD? Is it supported by csv-detective? In any case, should we test it?
There was a problem hiding this comment.
Yes this is supposed to be supported ! As well as YYYY/DD/MM
| failsafe=False, | ||
| ) | ||
| for _type, value in zip(columns.values(), line) | ||
| for (_type, date_format), value in zip(columns.values(), line) |
There was a problem hiding this comment.
This is becoming a bit difficult to read so I changed for a more explicit (and performant) unpacking: bb5d991
3 participants
Use the new
date_formatadded in datagouv/csv-detective#245