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

Format date field of time series #17

Closed
wants to merge 1 commit into from
Closed

Format date field of time series #17

wants to merge 1 commit into from

Conversation

msbit
Copy link

@msbit msbit commented Mar 19, 2020

Pad the start of the date field of time series to ensure the
dates are of the form:

YYYY-MM-DD

I've tested this on my repo with a per-minute action and it appears to all go through swimmingly.

Pad the start of the `date` field of time series to ensure the
dates are of the form:

    YYYY-MM-DD
@jeetiss
Copy link
Contributor

jeetiss commented Mar 19, 2020

This is breaking change and will destroy all projects that already use this data.

You can use https://github.com/rlindskog/covid19-graphql that support converting date format

@pomber
Copy link
Owner

pomber commented Mar 19, 2020

Hey @msbit, thanks for the PR.

I don't think it will destroy all projects that already use this data, but there's a possibility that it may break some projects.

Since this can be done on the client, any reason you prefer to do it in the GH action?

@pomber pomber mentioned this pull request Mar 19, 2020
@msbit
Copy link
Author

msbit commented Mar 20, 2020

@pomber

Since this can be done on the client

I'm guessing that you're referring to https://github.com/rlindskog/covid19-graphql ? If so, I wasn't aware of it until seeing @jeetiss comment.

any reason you prefer to do it in the GH action?

Well, mainly so the date is in a format that is more compatible and less likely to surprise consumers.

Either way, no stress if this PR doesn't fit with this project :)

@pomber
Copy link
Owner

pomber commented Mar 20, 2020

I'm guessing that you're referring to https://github.com/rlindskog/covid19-graphql ? If so, I wasn't aware of it until seeing @jeetiss comment.

I'm not referring to the graphql api. I mean it can be done from any consumer of the json, for example, the javascript fetching the json.

I agree that your change makes a better date format, but I should have format it like that before people started using it. Now, making a breaking change (even small like this) isn't worth it.

Thanks again!

@msbit
Copy link
Author

msbit commented Mar 20, 2020

@pomber

I mean it can be done from any consumer of the json

Ah okay, this might get a little philosophical!

I've always thought that if it can be, a problem should be fixed upstream. In that way, it doesn't need to be done from any of the greater-than-one consumers of the JSON, so the overall workload across all projects is reduced. As a counterpoint, you might say that some of your consumers prefer the format that you are currently outputting. To that, I'd say every choice we make as developers is opinionated, and so will never make everyone happy; the best we can achieve, where there is no clear advantage to any, is to follow the general consensus, to create the least surprise for those that use the software. In this case, I feel we are in agreement that the format I'd proposed was closer to that general consensus.

Now, having said all that:

Now, making a breaking change (even small like this) isn't worth it.

Totally understand, feel free to close this PR

:)

@pomber pomber closed this Mar 20, 2020
@pomber pomber mentioned this pull request Apr 17, 2020
@saschawildgrube
Copy link

It's a bug in my eyes. Bugs need to be fixed. If you're worried about downstream effects, create two files going forward.

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

4 participants