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

Doing a first aim at getting midnight after adding 1 day. If that tim… #3716

Closed
wants to merge 1 commit into from

Conversation

keithwillcode
Copy link

…e is not midnight, doing a second aim at midnight.

Worked on this with @kevPo.

@jsf-clabot
Copy link

jsf-clabot commented Jan 13, 2017

CLA assistant check
All committers have signed the CLA.

…e is not midnight, doing a second aim at midnight.
@keithwillcode
Copy link
Author

Closes #3132

@keithwillcode
Copy link
Author

@ichernev Both grunt and the transpile commands are completing successfully on my machine. Thoughts on why they would fail on Travis CI?

@keithwillcode
Copy link
Author

@mj1856 ^^

@ichernev
Copy link
Contributor

ichernev commented Feb 4, 2017

@keithwillcode relax, software is sometimes (sigh) buggy.
About the code itself -- I didn't quite get that.

  • shouldn't we be careful with this logic for units smaller than a day?
  • doing another startOf might not be great, if you're off because of DST hole then you should try to find which way to go (fw or bw) depending on which one is closer (now you go only back)
  • also you're trying to force America/Sao_Paulo in your tests, but that only works in moment-timezone, so you can not use this. You should introduce a DST hole with a function, so its always reproducible and doesn't needs dependencies -- https://github.com/moment/moment/blob/develop/src/test/moment/zones.js#L299

I hope the second aim will do it right ;-)

@maggiepint
Copy link
Member

closing due to lack of response. Feel free to reopen if concerns are addressed.

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