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

Make today/tomorrow/yesterday at the beginning of the day #14

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

aviadl
Copy link

@aviadl aviadl commented Jan 17, 2019

zero out hour/minutes/seconds when working with 'today/tomorrow/yesterday'

@dmitshur
Copy link

Issue #13 is related.

@tj
Copy link

tj commented Nov 19, 2019

👍 I think this makes sense as well, if you have something like "yesterday at 5pm", you'll still get the seconds of now, at least in my case (query range) that seems surprising.

Maybe an option to always zero anything that isn't specified?

@olebedev
Copy link
Owner

Well, it seems to be a case for plugging those rules you would like to use for a particular case. Example:

import (
	github.com/olebedev/when
	github.com/olebedev/when/rules/en
	github.com/olebedev/when/rules/common

	customENGRules github.com/demisto/when/rules/en // from the above
)
w := when.New(nil)
// this rule will match before the rest ones, the order is matters
w.Add(customENGRules.CasualDate)
// ... add whatever rules you want, like:
// w.Add(en.All...)
// w.Add(common.All...)

@tj, I think an option here is an unnecessarily in favor of using rules as building blocks for a case. Wdyt?

@olebedev olebedev self-requested a review November 19, 2019 22:00
Copy link
Owner

@olebedev olebedev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aviadl, I am happy to accept the rule you suggested but as an alternative rule which is not included in the default instance. Maybe a separate directory would be fine, like: rules/en/misc. And the README or Godoc with a description of the rule and examples, etc.

rules/en/casual_test.go Outdated Show resolved Hide resolved
Co-Authored-By: Oleg Lebedev <oleg@canva.com>
@aviadl
Copy link
Author

aviadl commented Nov 19, 2019

@aviadl, I am happy to accept the rule you suggested but as an alternative rule which is not included in the default instance. Maybe a separate directory would be fine, like: rules/en/misc. And the README or Godoc with a description of the rule and examples, etc.

Not sure why the original one is valid, but i will let you take it from here, and do the changes you believe in

@tj
Copy link

tj commented Nov 20, 2019

Custom rules sounds reasonable to me, I didn't look at how they're implemented yet but I'll take a look!

@olebedev
Copy link
Owner

olebedev commented May 31, 2023

Not sure why the original one is valid, but i will let you take it from here, and do the changes you believe in

Hello @aviadl,

Thank you for your contribution. I appreciate the effort you put into this matter. After reconsideration, I agree with the change you suggested for "valid." However, I have one concern remaining. Users who are already utilizing the library may encounter unexpected behavior when they update it.

I'm considering whether it would be beneficial to introduce versioning to the library. I would like to hear your thoughts on this matter.

Related #16.

Best regards,
Oleg

@aviadl
Copy link
Author

aviadl commented Jun 1, 2023

Not sure why the original one is valid, but i will let you take it from here, and do the changes you believe in

Hello @aviadl,

Thank you for your contribution. I appreciate the effort you put into this matter. After reconsideration, I agree with the change you suggested for "valid." However, I have one concern remaining. Users who are already utilizing the library may encounter unexpected behavior when they update it.

I'm considering whether it would be beneficial to introduce versioning to the library. I would like to hear your thoughts on this matter.

Related #16.

Best regards, Oleg

IMO i would treat the current behaviour as a bug, so i think that this fix is fine, and not need for versioning
we can also add a flag to the library whether to work this way or not, if you really are concerned about this

BTW - you can start working with git releases, and then it will change only for customers that take the latest release

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