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

Convert dates to datetimes for DateHourParameter #2285

Merged
merged 1 commit into from Nov 16, 2017

Conversation

mkennely
Copy link
Contributor

Description

Base class for DateHourParameter throws an error when a date instead of a datetime is passed as an argument. Code was added to convert dates to datetimes when passed to DateHourParameters in order to alleviate aforementioned issue.

Motivation and Context

The current Luigi set up I work with utilizes base tasks to avoid repeating code. Some processes use DateParameters and others use DateHourParameters and errors arise when trying to pass dates to DateHourParameters

Have you tested this? If so, how?

Unit test is included

@mkennely
Copy link
Contributor Author

Not real sure who to tag. So @Tarrasch @mikekap , could you review?

@Tarrasch
Copy link
Contributor

I consider it to be a type error, hence a crash makes sense to me. Not sure if I want a behavior change.


The code of the PR is very clean though. :)

@dlstadther
Copy link
Collaborator

It should be noted that DateParameter already does the opposite here - converts datetime objects to dates. Makes sense to me that datetime should be able to take a date and assume midnight. However, I can also see the type error desire too

@Tarrasch
Copy link
Contributor

Hmm... well I guess it kinda makes sense that you always round down to the lowest of granularity time. LGTM. :)

@dlstadther dlstadther merged commit 0777523 into spotify:master Nov 16, 2017
@mkennely mkennely deleted the date-param-fix branch November 16, 2017 12:52
This was referenced Jun 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants