search

KristjanESPERANTO / MagicMirror-3rd-Party-Modules

This project provides an overview of all MagicMirror² modules and puts the modules through a few tests.
https://kristjanesperanto.github.io/MagicMirror-3rd-Party-Modules/
MIT License
13 stars 4 forks source link

Date() Nuance #26

Open dathbe opened 3 months ago

dathbe commented 3 months ago

See this discussion for a nuance about the Date() recommendation offered by this repo: https://github.com/BKeyport/MMM-Multimonth/pull/21

It would be good if the recommendation engine was a little more context sensitive, but if that's too difficult (probably), then some additional guidance in the recommendation would be useful.

For example, it should probably say "Consider replacing" instead of "Replace". And I read the post 3252 that is linked, and did not follow what the actual issue is, so a bit more description of when and why you'd make the change would be helpful.

Again, thanks for your work on this.

KristjanESPERANTO commented 3 months ago

Yes, this is a change whose point is perhaps not so clear without an explanation.

new Date() and new Date(Date.now()) are basically the same. The advantage of the second is that Date.now() can be overridden to test the behavior of the module at other dates/times. This can be very helpful for debugging time related issues.

There is a PR for the MagicMirror documentation which addresses this. When it's merged, we can link to that part. Until then maybe don't do this change in your PRs.

For example, it should probably say "Consider replacing" instead of "Replace".

Good point. I just changed it :slightly_smiling_face:

Thanks for your work! I'm concentrating on other problems at the moment, so it's possible that things aren't progressing so quickly here. But don't let that hold you back from raising your questions and ideas.