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

Fix unittest and code cleanup #69

Merged
merged 1 commit into from
Jul 29, 2024
Merged

Conversation

sashacmc
Copy link
Contributor

@sashacmc sashacmc commented Jul 29, 2024

Copy link
Contributor

@gregmedd gregmedd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good - I don't see anything that is critical to change.

In the future, it might be helpful to split up multiple changes like this. For example, the PR could have contained a commit that just removed the using namespace statements, then another commit that fixed the test failure, and so on. That way a reviewer could look at each change individually if it makes the review easier.

Comment on lines +29 to +31
uprotocol::v1::UStatus ZenohUTransport::uError(uprotocol::v1::UCode code,
std::string_view message) {
uprotocol::v1::UStatus status;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: since this code is already inside the uprotocol namespace, the uprotocol:: prefix isn't required. For example, this could be:

Suggested change
uprotocol::v1::UStatus ZenohUTransport::uError(uprotocol::v1::UCode code,
std::string_view message) {
uprotocol::v1::UStatus status;
v1::UStatus ZenohUTransport::uError(v1::UCode code,
std::string_view message) {
v1::UStatus status;

@gregmedd gregmedd merged commit 9499a4c into eclipse-uprotocol:main Jul 29, 2024
5 checks passed
@gregmedd gregmedd linked an issue Jul 30, 2024 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement and test transport::ZenohUTransport
2 participants