Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Fix compatibility with linux #2
Fix compatibility with linux #2
Changes from 5 commits
f3cd095
791ccde
152cb81
8faa681
37125eb
10d49e6
aa068d8
d81e009
cd6793f
5e5a4ec
cb1a23c
9c9359e
df5633e
3926ae5
abf5c1d
6c1945d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
No comma before if. "... is fine if the user ..."
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.
docker -> Docker, and no comma after "Docker"
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.
no comma after "i.e."
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.
just a curiosity on that one: it depends on the style used; American style encloses the abbreviation within commas and the British style does not
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 was told many times by a native English speaker that whenever possible, use the active and not the passive voice. In this case, that would mean instead of "can be accessible by the Docker container" (passive) we should rather write "Docker can directly access".
In German, we often use the passive voice, but in English, it's better to use the other way.
I would write this sentence like this: "This way, Docker can directly access the virtual network interface created by the HomePlugGreenPHY module and it's possible to use the local-link address."
It's just easier to the English-speaking mind.
By the way: is HomePlugGreenPHY a software module you are referring to, or do you mean the actual HW device? If the latter, then we should write it as Home Plug Green PHY
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.
Both are widely used, so it will depend on what you want to emphasise, if the action itself (passive) or the subject of the action (active). In this case, I agree, active voice is a better choice here.
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.
Sice Switch team -> Since the Switch team
Also let's avoid using "since" twice in a sentence, you can just delete the second "since"
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.
No comma before "if"
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.
Do we really need to mention these MQTT variables in this project? If this project will eventually be used by people outside of Switch (and become open-source), with no access to our MQTT implementation, then we might as well omit this here and add the information in our Josev project. What do you think?
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.
We dont, I removed it already...thanks.
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.
docker -> Docker
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.
So there's a guaranteed link-local address? What if someone configures the wrong interface? I believe we still do need this exception, no?
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.
No, there is no guarantee. So, first we need to check if the interface specified exists, secondly, we need to check if the interface contains a link-local address. If any of those checks fail, we raise an error and we are not supposed to go on, i.e. the application is supposed to crash.
Before, the code would just check if we had provided an interface and would not even check if the interface actually existed (would also not check, in this case if the interface had a valid link-local address). Later on, in case the interface didn't exist, this would throw an error but wouldnt be completely explicit:
I modified the code to tackle that issue, but I realise now that it is not enough and we also need to check for the validity of the local link
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.
Due to the changes on handling exceptions I did some reworking on the UDP client and server services, as in some occasions (namely, when an invalid interface was selected) the code would throw the error:
This was happening, because the UDP server and client tasks were not being handled by the
wait_until_finished
, which cancels properly the tasks in the list of tasks.