openhab / openhab-vscode

VS Code extension for openHAB configuration files
https://marketplace.visualstudio.com/items?itemName=openhab.openhab
Eclipse Public License 2.0
159 stars 47 forks source link

Add human readable threadsleep description hover. #242

Closed Confectrician closed 3 years ago

Confectrician commented 3 years ago

Fixes #237 Fixes #240

Signed-off-by: Jerome Luckenbach github@luckenba.ch

Confectrician commented 3 years ago

@CWempe could you do some testing?

Extension with this changes included can be downloaded at our azure artifactory

CWempe commented 3 years ago

I used this test commands:

Thread::sleep(12345678)
Thread::sleep(1234567890)
Thread::sleep(1234567890123)
Thread::sleep(9765432)
Thread::sleep(7)
Thread::sleep(4000)
Thread::sleep(60000)
Thread::sleep(7abc)
Thread::sleep(7.)
Thread::sleep(xy7abc)

As expected, it does not calculate days or years. 😜

image

And a warning would be nice if there is an invalid value inside the method.

image

And it seems there is a big blank line above "Sleep Time".

Other than that it looks great!

Confectrician commented 3 years ago

I have limited the matching up to 9 digit millisecond numbers. This would theoretically allow a sleep of 11.5 days when using 999999999. Thinking about decreasing this to 7 digits, which would be about 2.7 hours. This is still way longer than a sleep should be for best practice.

Also i will not do any validation (at least like "you should not mix digits and numbers") in the hover. Validation should be done on language level and our hover provider works separate from other code validations currently. Anyway i will try to not display anything, when some of the mixed stuff, like shown above, gets hovered.

Confectrician commented 3 years ago

OK this changes should prevent the hover from being shown when something different than a 1 to 7 digit number is in the thread sleep method.

Vsix is available in our azure artifactory

Confectrician commented 3 years ago

Overall solution looks good so far. We can still adapt smaller improvements, therefore merging already. 🙂