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

Add UTC versions of TimeUnit #46

Closed
wants to merge 4 commits into from
Closed

Conversation

jeffesp
Copy link

@jeffesp jeffesp commented Mar 21, 2018

This isn't the prettiest change, but it seems to work in my local testing.

I'm unable to generate accurate graphs without it.

@jwoLondon
Copy link
Member

Thanks very much for suggesting this. I had meant to add UTC time units in an earlier release so your prompt has been helpful.

Rather than add a set of another 24 types to what is already rather a cumbersome set of time unit types, I would prefer to add a single Utc tag that takes any TimeUnit as a tag. For example,

 encoding
        << position X [ PName "date", PmType Temporal, PTimeUnit (Utc YearMonthDateHours) ] 

I've created a utc branch on the repo for you to have a look at and test. Would that work for you? If so, I can merge it into the master. Unfortunately, because we are forced to add to the TimeUnit type, this will bump the release as a major change, so ideally I'd like to wait to merge until other changes have been incorporated - specifically to have some Vega support in addition to Vega-Lite.

@kachkaev
Copy link
Member

Hi @jeffesp, thanks for this contribution!

I see you've started to make a fork with your functionality that contains a new url. etc. Could you please do it in a new branch (e.g. develop) and keep your master at 14f9ccc for now? This will make this PR mergable if @jwoLondon decides to 😉 It'd be better if there was an option to change PR's branch name after its creation so that you could use your master, but I guess GitHub does not support that.

@kachkaev
Copy link
Member

kachkaev commented Mar 23, 2018

Oops – commented simultaneously with @jwoLondon 😅

@jeffesp if you're fine with the new functionality in a different branch, no need to do anything – we'll simply close this PR later on.

@jeffesp
Copy link
Author

jeffesp commented Mar 23, 2018

@jwoLondon @kachkaev

The changes in the utc branch look great. I agree that the extra types were very cumbersome. Thanks for the prompt reply here and feel free to drop this merge request whenever.

@jwoLondon
Copy link
Member

OK, that's good to know - thanks. If you want to use the utc branch locally for now I don't have any plans for further changes to the API for dates in the next release, which realistically won't be for a few weeks yet as we work on other additions.

@jwoLondon jwoLondon closed this Mar 23, 2018
@jwoLondon
Copy link
Member

@jeffesp The latest release of elm-vega (2.2.0) now includes UTC support. It uses a function utc to convert any TimeUnit to its UTC equivalent, rather than a new type constructor. This means it can be released without a major version bump.

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.

3 participants