Bugfix/Workaround for issue #319: split group_value by ',' for replac…

[tboegner]

…ement by defined resources.

[ypid]

Hi @tboegner

Thanks for the patch. It works functionally but as I was reviewing and writing a test for it, I noticed that this bug occurred because the implementation is badly designed. Apparently I missed that when I reviewed/merged the feature in v3.3.0 (2015). Working with regex on the generated prettified_group_value that we just generated from the tokens does not make sense. In this function, we are in the nice position that the oh_value is fully parsed and we can do more semantic things than just stupid regex.

Can you update this patch and hook into prettifySelector instead? Bonus points when you refactor the existing function part that deals with locale also. You seem interested in this feature so doing it in prettifySelector should make it much more stable.

[tboegner]

Hi @ypid

I agree with you that this patch is not a nice solution. It is just a workaround for a problematic parsing result. The problem is, that the holiday token is not identified as "holiday" and is grouped together with the weekday. Afterwards the only possibility I see is splitting the group by the ',' and replacing the concerned parts.
In my opinion the clean solution should be made in getSelectorRange() to separate the holiday from the weekday. But this is also not easy since the holiday tokens don't have a significant type in the input data. The type is 'weekday' or undefined depending on the order of the tokens.

So I was not able to fix it in a clean way without spending too much time. It is absolutely OK, if you don't want to integrate the patch. Maybe there is a better solution in the future.

[ypid]

All right. I will leave the PR open then to track it. Thanks for spotting the bug and working on a patch :)

But this is also not easy since the holiday tokens don't have a significant type in the input data. The type is 'weekday' or undefined depending on the order of the tokens.

I agree. This is not so well designed in the spec. But it works in practice and I don’t think we can change it now.