andrewplummer / Sugar

A Javascript library for working with native objects.
https://sugarjs.com/
MIT License
4.53k stars 306 forks source link

Params contains incorrect specificity and is missing time parts in certain cases #569

Open johncrenshaw opened 7 years ago

johncrenshaw commented 7 years ago

The problem seems to happen specifically when a relative unit is used with other information that is more specific than the relative unit.

The easiest example is "yesterday at 2:30pm" which yields the following params: { ampm:1, day:-1, num:-1, specificity:4, unit:4 }

unit = 4 and num = -1 make sense, but specificity should be 2, and params should also have hour = 2 and minute = 30

andrewplummer commented 7 years ago

Out of curiosity, how did you come across the params feature? Was it through the documentation? This is a nearly hidden feature that was added as an afterthought, so I'm sure about how people are both using it and finding it.

That said, it seems like it needs to be given more attention.

andrewplummer commented 7 years ago

One issue that I had not foreseen when adding this option is the fact that params may be a mix of relative and absolute (your example is a good example of this)... in fact that's the core cause of this issue (relative params are set first, then absolute after, but are not being captured). If we are going to properly address this, maybe it's a good time to flesh out use cases, as I can see not having the ability to know which params are relative in ambiguous input being a problem.

Additionally, now that I look at it, the "specificity" parameter is essentially lying to you now, as this date is specific to minutes, not days.

andrewplummer commented 7 years ago

To be perfectly honest, "specificity" is more of an internal construct that wasn't intended to be exposed, so I'm not sure we should be doing that.

andrewplummer commented 7 years ago

That said, it is quite useful... One idea, what about another parameter like relativeIndex to match specificity (maybe specificityIndex is better?) that indicates up to which parameter is relative. It should always be the case that relative parameters are higher than absolute, and that after a switch to absolute it cannot go back to relative. Consider these formats:

etc. Having absolute parameters higher than relative ones should make no sense:

Additionally, for the same reason it should not be able to switch back after having gone absolute:

If we can be sure of these rules, then a relativeIndex should function well enough to capture the intent of any format.

johncrenshaw commented 7 years ago

Params is documented in the code and mentioned (under an older name as "set") briefly on a previous issue. I found it when looking for a way to get basically the exact information that this provides.

My use case is natural language processing. Because dates in a natural language setting are actually fuzzy time intervals and not absolute moments, it is important to be able to understand the granularity of a date. So, for example, "this week" and "Sunday at 12am" may both parsed to the exact same date and time value, but the granularity is very different. If you imagine a use case where you are trying to look up records based on that date it quickly becomes important to understand the granularity.

As for the "relativeIndex" idea, this seems to be provided already in the "unit", but I think that is separate from the information I need in my case.

I could also ignore specificity and and compute the granularity myself, except that units of a lower order than the specificity seem to not be reported in params either; so, in your "March of next year" example, params reports that there is a year, but fails to report that there is a month.

andrewplummer commented 7 years ago

Cool. That's a good use case, I was just curious about it.

So, unit does not have to do with the relative date, and is an internal parameter that should be removed, so this still needs to be fixed for relative times.

I could also ignore specificity and and compute the granularity myself

That seems extraneous, since Sugar already does it...it just needs to be exposed.

except that units of a lower order than the specificity seem to not be reported in params either

Right, so it's 2 separate concepts. There is an all around granularity (what was the most specific time specified), and the relative granularity (what was the most specific relative unit?). I think 2 params should take care of it, all that needs to be decided is the naming. For my part, I think specificityIndex and relativeIndex is the best. relativeIndex would only appear if there is a relative time involved.

dusty commented 6 years ago

I came across this today as well. I'm using it to parse the start and end time for an event.

Here is an example.

If params.hour is null on startTime, I will default this to an All Day event and behind the scenes set the full date to include the start of that day.

If params.hour is null on endTime, I will default to the end of the day.

Something like that.

I see this issue is pretty old, is this still being looked at? I can certainly work around this.

Thanks

andrewplummer commented 6 years ago

@dusty Sorry yes this issue has taken forever but it's finally being addressed. The bug that you're dealing with has been fixed and I will take a look at relativeIndex or something like it for the next major version. Of course I'm sure you're not holding your breath at this point....