-
-
Notifications
You must be signed in to change notification settings - Fork 238
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 more datetime support, type-independent log block , CRLF block #1382
add more datetime support, type-independent log block , CRLF block #1382
Conversation
…d OH section Signed-off-by: Stefan Höhn <stefan.hoehn@nfon.com>
Job #409: Bundle Size — 10.74MB (+0.28%).Changed metrics (2/10)
|
Thank you Stefan, a few remarks as I was testing this PR: I'm not sure "ZDT" as a technical acronym would be understood by novices; there ought to be a more user-friendlier identifier for them Not sure what this does, I have some idea with my technical background but regular users will IMO surely be confused. |
Hi Yannick,
I agree on the ZDT-acronym. The reason I used it that I took over from deibich's implementation which you liked pretty well. How about replacing it with datetime or even zonedDatetime.
I actually showed this several people including @Confectrician in a live demo before completely implementing it. As soon as you we have merged this code I will document it and you will see it is straight forward and easy to use. How to use it
copyOfDemo.movOnce you get the hang of it it is really straight forward and pretty easy. I intend to add an animated gif (like above) to the docu showing how to use it |
Hi all, Jumping in here through the mention. Yes. There might be kind of a learning curve. But when you are a bit familiar with it, you can use one single block in a highly flexible way and just have to combine the same parts in a different way. |
I agree this is great stuff but I'm really still unconvinced about "ZDT" - it really doesn't mean anything for the casual user (and you can't expect casual users to go check out the openHAB docs to figure that out, I believe especially in Blockly things have to be approachable and naturally tailored for non-technical users) - however if it was spelled out completely as "ZonedDateTime" or even "zoned date time" then they could at least google it and learn about what they're dealing with. Just my opinion ;) Edit: turns out searching for "zoned date time" gives at least relevant results on:
So this would have my preference. |
Hm Stefan agreed in renaming already and is probably only waiting for feedback on his suggestions.
My 2 Cents: |
As mentioned above am fine with both date time and zoned date time. As date time is shorter I will change it that asap. |
Sorry I completely missed that part. I just was focused on the "add temporal units" block video ;) |
Signed-off-by: Stefan Höhn <stefan.hoehn@nfon.com>
@ghys As the next milestone is around the corner would it be possible for you (or anyone else) to merge my Pull Request? |
I'm currently abroad without a laptop, I'll finish the review over the weekend after I return! |
Signed-off-by: Yannick Schaus <github@schaus.net>
Signed-off-by: Yannick Schaus <github@schaus.net>
Signed-off-by: Yannick Schaus <github@schaus.net>
Signed-off-by: Yannick Schaus <github@schaus.net>
Thanks for being so rigid on the review (I looked at it) - I appreciate it. One question regarding
How does that work? ...strange that the JAVA-Build fails. |
Signed-off-by: Yannick Schaus <github@schaus.net>
Signed-off-by: Yannick Schaus <github@schaus.net>
I restored the original package.json / package-lock.json as I haven't been able to immediately fix the peer dependencies issues. I'll address that another time in another PR. As answered on the forum please refrain from updating unrelated dependencies and committing/submitting the result as a PR (or more generally anything unrelated to the PR itself), it avoids some headaches! :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm leaving some notes on the implementation of those big utility functions, and other remarks, but I won't ask you to address those at least not in this PR.
case 'getMicro': | ||
temporalPart = `getLong(${chrono}.MICRO_OF_SECOND) % 1000` | ||
break | ||
case 'getNano': | ||
temporalPart = `getLong(${chrono}.NANO_OF_SECOND) % 1000` | ||
break |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't quite understand why the % 1000
for micro & nano. For nano I seem to always get 0.
In fact I wonder if anything below a millisecond is really necessary, who would ever need them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First of all I didn't want to deviate from deibich's implementation as he had implemented nanos (and yes, you could of course question whether nanos are really necessary). I then went further because millis and nanos where there but micros was missing, so for consistency reasons I have added them.
Regarding your note I checked the behaviour with
which results into
2022-05-30T11:34:26.855407+02:00[Europe/Berlin]
2022-05-30T11:34:26.855407123+02:00[Europe/Berlin]
123
So, you are right that now does not include Nanos itself, so we could have omitted them. Sure, you can set and add and subtract them but the question really is how much sense that makes.
Would you vote for removing nanos everywhere? I would then remove them with another PR.
' stringToParse += \':\' + (\'0\' + minute).slice(-2);', | ||
' stringToParse += \':\' + (\'0\' + second).slice(-2);', | ||
' stringToParse += \'.\' + nano + offsetString + \'[\' + timezoneString + \']\';', | ||
` return ${zdt}.parse(stringToParse, dtf.ISO_ZONED_DATE_TIME);`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to be an implementation of ZonedDateTime.of
so why not use it directly?
Since these functions are injected when you inject date support whether you need to call them of not, and are quite long, they tend to clutter users' scripts very quickly, so the fewer, the better. Besides we should IMHO use the best possible reference implementation when injecting things into user scripts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good hint.
so why not use it directly?
for two reasons:
- I took that over from deibichs implementation directly
- I wasn't aware of that zdt.of
I will see if I can optimize that eventually.
export function addDateComparisonSupport () { | ||
let zdtCompare = Blockly.JavaScript.provideFunction_( | ||
'zdtCompare', [ | ||
'function ' + Blockly.JavaScript.FUNCTION_NAME_PLACEHOLDER_ + '(zdt1, zdt2, compareOp, precision, compDate) {', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly, instead of injecting this huge utility function, since those compareOp/precision/compDate are fields, you could generate the code dynamically, using temporary variables to store copies of zdt1
& zdt2
with the "precision" and "compDate" logic, then generate the code that you need dynamically.
In other words this big switchs/cases utility function should have been implemented in the code generation for the blocks instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will add that to my todo list and see what I can do here.
Note: set item from deibichs implementation was not ported as this can be done with a normal "send command"
2) OH Blocks are expanded by default
3) Logging-Block allows any input type
Also-by: deibich
Signed-off-by: Stefan Höhn stefan.hoehn@nfon.com
Big thanks goes to @tweini within the blockly community.