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
Make the trigger of the DDS Pipe callbacks configurable #67
Make the trigger of the DDS Pipe callbacks configurable #67
Changes from 10 commits
952d207
13c1126
e9b69f1
412105a
3dee011
b92a08b
83339d5
8197b79
2b89a35
8975877
8d8ded7
af12bb6
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.
Use lower case to simplify your life (and reviewer's).
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.
"Example is not another way to teach, it's the only way." — Albert Einstein
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.
Besides, it wouldn't simplify anything. We would have to rename the enum and all its occurrences to avoid copying an int. And in the future
to_uppercase
andto_lowercase
would return the string in uppercase or lowercase anyway.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.
"The fool knows after he's suffered." — Hesiod
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.
This makes little sense to me, as this logic specific to the remove unused entities feature is not supported for a discovery trigger value other than
READER
. But ok, it will need to be this way if we ever add support. However, I think it would be good to fail (add condition to someis_valid
) if an unsupported configuration is provided.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.
Related suggestion, although does not tackle the previous comment (cannot hold it back): I suggest having an if-else with
remove_unused_entities
withinis_relevant
method, and returnendpoint.active
when false.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 think it is more appropriate to address that change in a new branch, since it changes the dynamic of the DdsPipe at it should be thoroughly thought about and tested. I will also take the liberty to change other things which I think would improve the DdsPipe readability.
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.
Write an error trace in is_valid
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.
"Different to" is apparently colloquial, use "from" or "than".
https://dictionary.cambridge.org/es/gramatica/gramatica-britanica/different-from-different-to-or-different-than
https://www.wordreference.com/es/translation.asp?tranword=different%20to