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 date_in_tz filter. #73

Merged
merged 5 commits into from Apr 1, 2017
Merged

Conversation

djwf
Copy link
Contributor

@djwf djwf commented Feb 17, 2017

I wanted to add a date filter that would allow setting the timezone, so I created this filter based on the date filter. There is a lot of duplication between this filter and the date filter, but I decided to keep things simple for now.

Example use is: {{ "13 Jun 2016 12:00:00 +0000" | date_in_tz: "%Y-%m-%d",-13 }}, which would result in 2016-06-12.

@djwf
Copy link
Contributor Author

djwf commented Feb 18, 2017

@johannhof: I've just finished (I think) the reverse tag as well: shall I put each filter in a new pull request? Or all filters into one?

@djwf djwf mentioned this pull request Feb 18, 2017
@johannhof
Copy link
Contributor

@djwf depends on your confidence in your implementation, the advantage with individual PRs is that it's easier to contain review issues, but I think doing one big PR should be fine in this case.

Thanks for your contribution! I assume I can close this in favor of #74?

@johannhof johannhof closed this Feb 18, 2017
@johannhof
Copy link
Contributor

Oh actually reading the other PR it's probably better to review this one first. Apologies!

@johannhof johannhof reopened this Feb 18, 2017
@johannhof
Copy link
Contributor

Ok this looks great, thanks. (I love how Rust + Clippy + rustfmt make reviewing so easy)

One thing I'm a bit worried about would be adding too many (undocumented) filters to this. It's obviously great that people are contributing the filters they need but maybe we should hide all non-standard features behind a compiler flag. That could even be enabled by default but if someone really only wants the pure liquid experience they could disable the "extra-filters" feature.

Can you please hide this behind a feature flag and put that feature in the defaults? You can find examples how to do that here: https://doc.rust-lang.org/book/conditional-compilation.html

Thanks!

@johannhof
Copy link
Contributor

Oh and thanks for adding so many tests to it!

- Split tests out into separate functions following the most common pattern
  present in the existing tests.
- Cause incorrect type for second argument to return InvalidArgument, not
  InvalidType.
- Add test for incorrect type of timezone offset.
@djwf
Copy link
Contributor Author

djwf commented Mar 19, 2017

I added an "extra-filters" feature and set things up so that date_in_tz is included in that feature - thanks for the pointer to the howto. I think this handles all the issues, but let me know if you've any others!

@djwf
Copy link
Contributor Author

djwf commented Mar 19, 2017

@johannhof I looked through the failed builds, but I'm not sure what to do to fix them. Let me know if it's something I did.

@johannhof
Copy link
Contributor

Uh, good question. I retriggered it. :)

@djwf
Copy link
Contributor Author

djwf commented Mar 21, 2017

Thanks! However, it looks like something is broken in the appveyor builds, because now it can't even find curl.

@djwf
Copy link
Contributor Author

djwf commented Mar 24, 2017

@johannhof Any idea what might be causing the fail? I don't think the code is bad...

@epage
Copy link
Member

epage commented Mar 29, 2017

I patched AppVeyor for cobalt.rs, I guess this needs the patch as well. I hope later today I can fix that

@epage
Copy link
Member

epage commented Mar 31, 2017

Sorry for the delay, got my patch in. Travis will fail though on nightly because of clippy being broken again.

@djwf
Copy link
Contributor Author

djwf commented Mar 31, 2017

@epage thank you! @johannhof could you kick off another test and/or merge this? I'd like to work on some other stuff but don't want to lose track of this.

@epage
Copy link
Member

epage commented Mar 31, 2017

One thing I'm a bit worried about would be adding too many (undocumented) filters to this. It's obviously great that people are contributing the filters they need but maybe we should hide all non-standard features behind a compiler flag. That could even be enabled by default but if someone really only wants the pure liquid experience they could disable the "extra-filters" feature.
Can you please hide this behind a feature flag and put that feature in the defaults? You can find examples how to do that here: https://doc.rust-lang.org/book/conditional-compilation.html

@johannhof can we get your recommendation on this documented in CONTRIBUTING so we set expectations (and remember them) when other people contribute features?

@johannhof
Copy link
Contributor

Good idea! We should also document this flag in the README

could you kick off another test and/or merge this? I'd like to work on some other stuff but don't want to lose track of this.

I added you to the org, you should have kick-off-build rights on AppVeyor now (I also triggered a build though).

@johannhof
Copy link
Contributor

Errors because of a caching problem. Should be good to go.

@johannhof johannhof merged commit 2685a30 into cobalt-org:master Apr 1, 2017
@djwf
Copy link
Contributor Author

djwf commented Apr 1, 2017

Thanks! I'm traveling now, but will start the next project from the needs-help section this week.

@djwf djwf deleted the date-in-tz-filter branch April 1, 2017 21:07
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