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

[#18] Adds formatting for services and an additional example #44

Closed

Conversation

hydroid7
Copy link
Contributor

@hydroid7 hydroid7 commented Dec 15, 2023

Notes for Reviewer

Here is the pull request with fixed branch name and better formatting.

Pre-Review Checklist for the PR Author

  1. Add sensible notes for the reviewer
  2. PR title is short, expressive and meaningful
  3. Relevant issues are linked
  4. Every source code file has a copyright header with SPDX-License-Identifier: Apache-2.0 OR MIT
  5. Branch follows the naming format (iox2-123-introduce-posix-ipc-example)
  6. Commits messages are according to this guideline
  7. Tests follow the best practice for testing
  8. Changelog updated in the unreleased section including API breaking changes
  9. Assign PR to reviewer
  10. All checks have passed (except task-list-completed)

Checklist for the PR Reviewer

  • Commits are properly organized and messages are according to the guideline
  • Unit tests have been written for new behavior
  • Public API is documented
  • PR title describes the changes

Post-review Checklist for the PR Author

  1. All open points are addressed and tracked via issues

References

Use either 'Closes #123' or 'Relates to #123' to reference the corresponding issue.

Closes #29

@hydroid7 hydroid7 changed the title Adds formatting for services and an additional example [#29] Adds formatting for services and an additional example Dec 15, 2023
Copy link
Contributor

@elfenpiff elfenpiff left a comment

Choose a reason for hiding this comment

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

Thank you very much for the contribution 😄

Could you please adjust the commit messages and add [#29] as prefix to every commit message.
You can do this by calling git rebase -i HEAD~3, set the commit in editor from pick to edit, adjust the comment with git commit --amend and then call git rebase --continue.

This you can then push with git push -f.

When this is done and the comments are addressed we can merge this thing.

And welcome to the team, you are our first external contributor!

examples/examples/pretty_print.rs Outdated Show resolved Hide resolved
@hydroid7 hydroid7 marked this pull request as ready for review December 16, 2023 21:01
Copy link
Contributor

@elfenpiff elfenpiff 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. Could you please also add the Display implementation to the port factory of the publish subscribe messaging pattern in src/service/port_factory/publish_subscribe.rs.

@@ -127,3 +130,53 @@ impl<'config, Service: service::Details<'config>> PortFactory<'config, Service>
PortFactoryListener { factory: self }
}
}

impl<'config> fmt::Display for PortFactory<'config, zero_copy::Service<'config>> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be better to introduce the second generic parameter here: Service: service::Details<'config> and then replace the line f.debug_struct("zero_copy::Service") (136) with

f.debug_struct(format!("{}", core::any::type_name::<Service>()))

With this we reduce the code duplication and when we introduce another service variant we do not have to add here a third version (which may is even forgotten).

@hydroid7 What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, core::any::type_name is a game changer here! 🎉

}
}

impl<'config> fmt::Display for PortFactory<'config, process_local::Service<'config>> {
Copy link
Contributor

Choose a reason for hiding this comment

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

@hydroid7 If you follow my suggestion from above we can remove this here.

@elfenpiff
Copy link
Contributor

@hydroid7 could you please also add this into the doc/release-notes/iceoryx2-unreleased.md release documentation under the features section.

The entry would look somehow like this:

* Add `Display` implementation for `PortFactory` [#29](https://github.com/eclipse-iceoryx/iceoryx2/issues/29)

Copy link

codecov bot commented Dec 17, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (26d979e) 77.97% compared to head (83cb2eb) 78.14%.
Report is 6 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #44      +/-   ##
==========================================
+ Coverage   77.97%   78.14%   +0.16%     
==========================================
  Files         172      172              
  Lines       18729    18803      +74     
==========================================
+ Hits        14604    14693      +89     
+ Misses       4125     4110      -15     
Files Coverage Δ
iceoryx2/src/service/port_factory/event.rs 98.21% <100.00%> (+14.21%) ⬆️
...ryx2/src/service/port_factory/publish_subscribe.rs 100.00% <100.00%> (+10.00%) ⬆️

... and 3 files with indirect coverage changes

@elBoberido
Copy link
Member

I did not review the patch but please don't forget why this is done. We want to replace the custom printing code in these to locations

Pleas change those examples to just use println!("{}", pubsub); and println!("{}", event);

Oh, and the PR and commit messages use the wrong issue number. It's actually #18. #29 was the previous PR.

@hydroid7 hydroid7 changed the title [#29] Adds formatting for services and an additional example [#18] Adds formatting for services and an additional example Dec 19, 2023
@hydroid7
Copy link
Contributor Author

Thank you all for the constructive comments, I rewrote the commit messages, added the feature to the releasenotes, changed the documentation in order to reflect the new Display implementation and added the same for pubsub services.

Copy link
Member

@elBoberido elBoberido left a comment

Choose a reason for hiding this comment

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

Only had a look at the release notes. I'll leave the actual code to @elfenpiff

@@ -6,7 +6,7 @@

### Features

* Example [#1](https://github.com/larry-robotics/iceoryx2/issues/1)
* Add `Display` implementation for `PortFactory` [#29](https://github.com/eclipse-iceoryx/iceoryx2/issues/18)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Add `Display` implementation for `PortFactory` [#29](https://github.com/eclipse-iceoryx/iceoryx2/issues/18)
* Add `Display` implementation for `PortFactory` [#18](https://github.com/eclipse-iceoryx/iceoryx2/issues/18)

Copy link
Contributor

@elfenpiff elfenpiff 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, as soon as the issue number in the release notes is corrected we can merge this.

@hydroid7
Copy link
Contributor Author

Sorry that these took that long, but here they are.

Background for my contributions is that I'm writing a DDS middleware / message broker as my MSc Thesis.
Therefore I was first looking at iceoryx, but it was rather hard to use for someone without extensive knowledge in the project.

Copy link
Member

@elBoberido elBoberido left a comment

Choose a reason for hiding this comment

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

Please add [#18] here, similar to the other PR.

@elBoberido
Copy link
Member

@hydroid7 you need to fix the clippy warnings to make the CI happy

@elBoberido
Copy link
Member

@hydroid7 do you plan to finish this PR in the near future?

@elfenpiff
Copy link
Contributor

I think the PR is abandoned for now. @hydroid7 if you would like to continue working on this please feel free to reopen the PR again.

@elfenpiff elfenpiff closed this Aug 9, 2024
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.

iceoryx2-cal::hash Introduce uuid type and make it convertable to FileName
3 participants