Skip to content
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

fix: parsing arrows #336

Merged
merged 8 commits into from
Nov 29, 2024
Merged

fix: parsing arrows #336

merged 8 commits into from
Nov 29, 2024

Conversation

datawhores
Copy link
Collaborator

--completed-after and --completed-before use arrow parsing, and should accept strings so values like
2023.10.15 can be be input

Arrow will raise an expection if t can't parse a str

@NTFSvolume
Copy link
Collaborator

What is the benefit of using arrow over a regular datetime? Asking because pydantic has native support for datetime which accepts both int and str as input

It can even be restricted with the PastDate and BeforeDate annotations, to make sure the user can not input a 2028 date in the --complete-after for example

@NTFSvolume NTFSvolume added the bug Something isn't working label Nov 29, 2024
@datawhores
Copy link
Collaborator Author

datawhores commented Nov 29, 2024

Yeah I usually like to use arrow, because it the interface is a lot less confusing. And it has fewer object types. Most of what you want can be done within the main arrow module, while python has numerous modules

I wasn't able to find a datetime equivalent to arrow.get which accepts a date string and also an int. I'm not sure why I ran into issues, but arrow won't accept a numeric strings. So I'm guessing that is why.

However, pydantic seems to be modifield to handle a date str like arrow
https://docs.pydantic.dev/2.0/usage/types/datetime/. Arrow does support a more wide variety of date strs, but as long as pydantic is allowed to handle the parsing of dates. I think it is okay to use that.
The only thing is a date needs to be provided so there should be a default value or some way to set a date object before query. epoch 0 for --completed-after and the current date for completed-before

 completed_after: date| None = Field(default=0,description="only download completed downloads at or after this date")
completed_before: date | None = Field(description="only download completed downloads at or before this date",default=datetime.now())

Arrow objects can be converted into date and datetime, so it might be useful if we need to parse inputs from the database etc

arrow.get(item_date).date()

@NTFSvolume
Copy link
Collaborator

Pydantic will handle the conversion to date, no matter if it is a str or an int. They do need a default value but pydantic does not verify nor convert the default values to the expected type. They should be a date object already.

completed_after: date = Field(description="only download completed downloads at or after this date",default=date.fromtimestamp(0))
completed_before: date | None = Field(description="only download completed downloads at or before this date",default=None)

For completed_before the default should be None. You can check if it is None and them override it with datetime.now() at runtime before using it.

This is necessary cause using it inside the model means the default will change every time CDL runs, but the first time you run it, it will be saved to the config file.

That means any future run will use the value from the config, not the current default from the model.

@NTFSvolume
Copy link
Collaborator

Nevermind. Didn't realize these where CLI only arguments. It's fine is the both have a default value. They are never written to a configuration file

@datawhores
Copy link
Collaborator Author

datawhores commented Nov 29, 2024

I made some changes
Use datetime.date where possible

Removed the default from the Fields
edit: I saw your update, either one works. I don't have a preference on default or not

@NTFSvolume NTFSvolume merged commit 4efd392 into jbsparrow:master Nov 29, 2024
3 checks passed
@datawhores datawhores deleted the pydantic-arrow branch November 29, 2024 20:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants