-
Notifications
You must be signed in to change notification settings - Fork 90
refactor: avoid starting servient multiple times #1195
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
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #1195 +/- ##
==========================================
- Coverage 77.59% 77.57% -0.02%
==========================================
Files 83 83
Lines 17311 17332 +21
Branches 1747 1751 +4
==========================================
+ Hits 13433 13446 +13
- Misses 3842 3850 +8
Partials 36 36 ☔ View full report in Codecov by Sentry. |
JKRhb
left a comment
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.
Thank you, @danielpeintner, the changes already look good to me :) See two comments/suggestions below.
I do not see a use case but I do not see issues for allowing that |
The issues I see for allowing to restart is that I fear internal states "hanging around" that cause problems in the end 🙃 |
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 not sure I like the approach (should I throw or should I log and silently return). What about:
- In the start method:
- throw if the servient was shut down
- silently return and
debugmessage the servient was already started if it#startedis true
- In the shutdown method:
- throw if the servient was not even started (it's an invalid state after all)
- silently return and
debugmessage the servient was already shut down#shudownis true
Btw I'm okay with using #shutdown and #started but should we add the private modifier or is it still redundant (given they are really private fields for js)?
|
Thank you. I wasn't sure either.
I felt it is best to throw if it has been started already since I guess this is an actual error and as a developer I want to know it.
Good point... the more I think about I would do the opposite:
Do you have an other opinion? Throw in any case?
It is redundant and also shows as such in VS Code. |
|
In my perspective, we should throw if we are modifying the application state into something erroneous. So given the PR scope, the faulty state is when:
The others should not penalize the developer because:
Why do developers want to know if the method is called twice or more? |
|
I am mostly okay with your reasoning. There is one aspect that is a bit undesirable
The I am fine with doing so. |
|
Note: with the lastest changes we have issues with CoAP tests (see should reject requests for undefined meta operations). |
|
I think we should now have a solution that works for all of us ;-) |
relu91
left a comment
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 to go 👍🏻
I think it should not be possible to start a servient multiple times. Hence I added a simple check.
Moreover, I think a servient should be usable only once. Hence after a
shuthownit is no longer usable.Anyhow, I am open to other opinions whether we should allow for a re-
startagain?fixes #1193