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

[20543] Examples refactor: Hello World #4547

Merged
merged 42 commits into from
May 21, 2024

Conversation

JesusPoderoso
Copy link
Contributor

@JesusPoderoso JesusPoderoso commented Mar 11, 2024

Description

This PR should be merge right after:

This PR is the first of a suite of PR which would make a refactor in the repository examples.
It is intended to apply to most of the examples, by making them homogeneous, more understandable, and more specific to the case they were meant to be.

In this hello world example, the key changes are:

  • remove --env option and set that environment behavior as default
  • add subscriber waitsets class and its usage
  • provide XML profiles examples that target several scenarios (other tests as SampleConfig_Controller / Events / Multimedia)

The following changes apply to this and the remain examples:

  • remove the DDS prefix from the binaries
  • snake case in binary names and directory names
  • launch only one endpoint per binary (always running them in different shells)
  • unify args names and utility
  • write proper README.md with the purpose of the example and expected inputs, outputs, behavior… (do not include options in readme → enhance user to run --help )
  • implement listener callback in the same class (inheritance)
  • include initialization ( init() ) methods in the constructors
  • improve parsing options to be a black box to the user

Contributor Checklist

  • Commit messages follow the project guidelines.
  • The code follows the style guidelines of this project.
  • Tests that thoroughly check the new feature have been added/Regression tests checking the bug and its fix have been added; the added tests pass locally
  • Any new/modified methods have been properly documented using Doxygen.
  • Changes are ABI compatible.
  • Changes are API compatible.
  • New feature has been added to the versions.md file (if applicable).
  • N/A New feature has been documented/Current behavior is correctly described in the documentation.
  • N/A Applicable backports have been included in the description.

Reviewer Checklist

  • The PR has a milestone assigned.
  • The title and description correctly express the PR's purpose.
  • Check contributor checklist is correct.
  • Check CI results: changes do not issue any warning.
  • Check CI results: failing tests are unrelated with the changes.

@JesusPoderoso JesusPoderoso added the ci-pending PR which CI is running label Mar 11, 2024
@JesusPoderoso JesusPoderoso added this to the v3.0.0 milestone Mar 11, 2024
examples/cpp/hello_world/cli_options.hpp Outdated Show resolved Hide resolved
examples/cpp/hello_world/cli_options.hpp Outdated Show resolved Hide resolved
test/examples/hello_world.compose.yml Outdated Show resolved Hide resolved
examples/cpp/hello_world/HelloWorldPublisher.cpp Outdated Show resolved Hide resolved
examples/cpp/hello_world/HelloWorldPublisher.cpp Outdated Show resolved Hide resolved
test/examples/hello_world.compose.yml Outdated Show resolved Hide resolved
test/examples/test_examples.py Outdated Show resolved Hide resolved
test/examples/test_examples.py Outdated Show resolved Hide resolved
examples/cpp/hello_world/CMakeLists.txt Outdated Show resolved Hide resolved
examples/cpp/hello_world/HelloWorld.idl Show resolved Hide resolved
@EduPonz EduPonz added the temporarily-blocked PR must be merged after another one label Mar 12, 2024
@JesusPoderoso JesusPoderoso force-pushed the feature/consider_profile_domain_id branch from 48f5bad to bb375a4 Compare March 12, 2024 11:06
@JesusPoderoso JesusPoderoso force-pushed the feature/example_refactor/hello_world branch from c366a69 to 6f2a1e0 Compare March 12, 2024 16:20
@JesusPoderoso JesusPoderoso force-pushed the feature/consider_profile_domain_id branch 2 times, most recently from 7af5a31 to d73f963 Compare March 13, 2024 07:33
@JesusPoderoso JesusPoderoso force-pushed the feature/example_refactor/hello_world branch from 6f2a1e0 to c6e0741 Compare March 13, 2024 09:29
@JesusPoderoso JesusPoderoso force-pushed the feature/consider_profile_domain_id branch from d73f963 to 2dc171a Compare March 14, 2024 08:04
@JesusPoderoso JesusPoderoso force-pushed the feature/example_refactor/hello_world branch from 3cd6aa0 to 30317aa Compare March 14, 2024 08:11
examples/cpp/hello_world/CLIParser.hpp Outdated Show resolved Hide resolved
examples/cpp/hello_world/CLIParser.hpp Show resolved Hide resolved
examples/cpp/hello_world/CLIParser.hpp Outdated Show resolved Hide resolved
examples/cpp/hello_world/CLIParser.hpp Outdated Show resolved Hide resolved
examples/cpp/hello_world/CLIParser.hpp Outdated Show resolved Hide resolved
examples/cpp/hello_world/Subscriber.cpp Outdated Show resolved Hide resolved
examples/cpp/hello_world/SubscriberWaitset.hpp Outdated Show resolved Hide resolved
examples/cpp/hello_world/SubscriberWaitset.cpp Outdated Show resolved Hide resolved
examples/cpp/hello_world/SubscriberWaitset.cpp Outdated Show resolved Hide resolved
test/examples/test_examples.py Show resolved Hide resolved
@JesusPoderoso JesusPoderoso force-pushed the feature/example_refactor/hello_world branch from 813cb7d to fe33b04 Compare March 14, 2024 15:39
examples/cpp/hello_world/CLIParser.hpp Outdated Show resolved Hide resolved
examples/cpp/hello_world/CLIParser.hpp Show resolved Hide resolved
examples/cpp/hello_world/CLIParser.hpp Outdated Show resolved Hide resolved
examples/cpp/hello_world/CLIParser.hpp Outdated Show resolved Hide resolved
examples/cpp/hello_world/CLIParser.hpp Outdated Show resolved Hide resolved
examples/cpp/hello_world/CLIParser.hpp Outdated Show resolved Hide resolved
examples/cpp/hello_world/CMakeLists.txt Outdated Show resolved Hide resolved
Copy link

@EduPonz EduPonz left a comment

Choose a reason for hiding this comment

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

We're definitely getting there! I like the idea of the classes not having any static members.

Now, I have a "crazy" idea to simplify the main and to also group some of the subscribers' code. Right now, all three entities have similar public API:

  1. A ctor
  2. A run member function with no args
  3. A stop member function with no args

We can do some hierarchy of classes:

classDiagram
class Entity {
    +run() void
    +stop() void
   +create_entity(const hello_world_config& config)$ std::shared_ptr~Entity~
}
Entity <|-- Publisher
Entity <|-- AbstractSubscriber : Optional grouping of subscriber code
AbstractSubscriber <|-- Subscriber
AbstractSubscriber <|-- WaitSubscriber

Entity ..> Publisher : creates
Entity ..> Subscriber : creates
Entity ..> WaitSubscriber : creates
Loading

We would have a base class for the entities which can also act as a factory of the specializations via an static method. This way you don't need all those pointers, just the one, nor do you need any switches. You'd just create the entity passing the config and then spawn the thread to run it's run method. Since the entity is a shared_ptr, you wouldn't need to delete it either. You can see a PoC here.

examples/cpp/hello_world/HelloWorld_main.cpp Outdated Show resolved Hide resolved
examples/cpp/hello_world/Publisher.cpp Outdated Show resolved Hide resolved
examples/cpp/hello_world/Publisher.cpp Outdated Show resolved Hide resolved
examples/cpp/hello_world/Publisher.cpp Outdated Show resolved Hide resolved
examples/cpp/hello_world/Publisher.cpp Outdated Show resolved Hide resolved
examples/cpp/hello_world/HelloWorld_main.cpp Outdated Show resolved Hide resolved
@Mario-DL
Copy link
Member

We're definitely getting there! I like the idea of the classes not having any static members.

Now, I have a "crazy" idea to simplify the main and to also group some of the subscribers' code. Right now, all three entities have similar public API:

  1. A ctor
  2. A run member function with no args
  3. A stop member function with no args

We can do some hierarchy of classes:

classDiagram
class Entity {
    +run() void
    +stop() void
   +create_entity(const hello_world_config& config)$ std::shared_ptr~Entity~
}
Entity <|-- Publisher
Entity <|-- AbstractSubscriber : Optional grouping of subscriber code
AbstractSubscriber <|-- Subscriber
AbstractSubscriber <|-- WaitSubscriber

Entity ..> Publisher : creates
Entity ..> Subscriber : creates
Entity ..> WaitSubscriber : creates
Loading

We would have a base class for the entities which can also act as a factory of the specializations via an static method. This way you don't need all those pointers, just the one, nor do you need any switches. You'd just create the entity passing the config and then spawn the thread to run it's run method. Since the entity is a shared_ptr, you wouldn't need to delete it either. You can see a PoC here.

(NIT) Apart from the internally agreed things I would call the creation method (or global free function) make_app() instead of create 😃

@JesusPoderoso JesusPoderoso force-pushed the feature/example_refactor/hello_world branch 5 times, most recently from b2a2839 to 5064ab8 Compare March 20, 2024 15:22
@EduPonz EduPonz added temporarily-blocked PR must be merged after another one and removed temporarily-blocked PR must be merged after another one labels Mar 20, 2024
examples/cpp/hello_world/main.cpp Outdated Show resolved Hide resolved
examples/cpp/hello_world/CMakeLists.txt Show resolved Hide resolved
examples/cpp/hello_world/main.cpp Outdated Show resolved Hide resolved
examples/cpp/hello_world/CLIParser.hpp Outdated Show resolved Hide resolved
Copy link

@EduPonz EduPonz left a comment

Choose a reason for hiding this comment

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

This is turning out nicely, great job! 🤖

examples/cpp/hello_world/CLIParser.hpp Outdated Show resolved Hide resolved
examples/cpp/hello_world/CLIParser.hpp Outdated Show resolved Hide resolved
examples/cpp/hello_world/CLIParser.hpp Outdated Show resolved Hide resolved
examples/cpp/hello_world/CLIParser.hpp Outdated Show resolved Hide resolved
examples/cpp/hello_world/CLIParser.hpp Outdated Show resolved Hide resolved
test/examples/test_examples.py Outdated Show resolved Hide resolved
examples/cpp/hello_world/Application.hpp Outdated Show resolved Hide resolved
examples/cpp/hello_world/WaitsetSubscriberApp.cpp Outdated Show resolved Hide resolved
examples/cpp/hello_world/ListenerSubscriberApp.cpp Outdated Show resolved Hide resolved
examples/cpp/hello_world/PublisherApp.cpp Outdated Show resolved Hide resolved
JesusPoderoso added a commit that referenced this pull request Mar 21, 2024
JesusPoderoso added a commit that referenced this pull request Mar 21, 2024
JesusPoderoso added a commit that referenced this pull request Mar 21, 2024
JesusPoderoso added a commit that referenced this pull request Mar 21, 2024
JesusPoderoso added a commit that referenced this pull request Mar 21, 2024
Signed-off-by: JesusPoderoso <[email protected]>
Signed-off-by: JesusPoderoso <[email protected]>
Signed-off-by: JesusPoderoso <[email protected]>
@JesusPoderoso JesusPoderoso force-pushed the feature/example_refactor/hello_world branch from fbafb51 to 416fce2 Compare May 20, 2024 14:39
@JesusPoderoso JesusPoderoso requested a review from EduPonz May 20, 2024 14:39
@JesusPoderoso JesusPoderoso removed the ready-to-merge Ready to be merged. CI and changes have been reviewed and approved. label May 20, 2024
@github-actions github-actions bot added the ci-pending PR which CI is running label May 20, 2024
@EduPonz EduPonz added ready-to-merge Ready to be merged. CI and changes have been reviewed and approved. and removed ci-pending PR which CI is running labels May 21, 2024
@EduPonz EduPonz merged commit e1770f0 into master May 21, 2024
9 of 12 checks passed
@EduPonz EduPonz deleted the feature/example_refactor/hello_world branch May 21, 2024 05:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge Ready to be merged. CI and changes have been reviewed and approved.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants