Skip to content
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 human readable threadsleep description hover. #242

Merged
merged 4 commits into from
Mar 16, 2021

Conversation

Confectrician
Copy link
Collaborator

@Confectrician Confectrician commented Mar 15, 2021

Fixes #237
Fixes #240

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

Signed-off-by: Jerome Luckenbach <github@luckenba.ch>
@Confectrician
Copy link
Collaborator Author

@CWempe could you do some testing?

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

@CWempe
Copy link
Contributor

CWempe commented Mar 15, 2021

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
Copy link
Collaborator Author

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.

Signed-off-by: Jerome Luckenbach <github@luckenba.ch>
Signed-off-by: Jerome Luckenbach <github@luckenba.ch>
@Confectrician Confectrician mentioned this pull request Mar 16, 2021
3 tasks
Signed-off-by: Jerome Luckenbach <github@luckenba.ch>
@Confectrician
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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

@Confectrician Confectrician merged commit 46fef5e into openhab:main Mar 16, 2021
@Confectrician Confectrician deleted the hover-provider branch March 16, 2021 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants