Skip to content

Count publishers and subscribers#183

Merged
sloretz merged 6 commits intoros2:masterfrom
Nickolaim:pubsub_count3
May 11, 2018
Merged

Count publishers and subscribers#183
sloretz merged 6 commits intoros2:masterfrom
Nickolaim:pubsub_count3

Conversation

@Nickolaim
Copy link
Copy Markdown
Contributor

@Nickolaim Nickolaim commented Apr 7, 2018 •
edited by wjwwood
Loading

This is a partial implementation of ros2/ros2cli#60. I marked it as a WIP since I expect some comments. Plan to merge changes later and do another PR.

Connects to ros2/ros2cli#60

@wjwwood
Copy link
Copy Markdown
Member

wjwwood commented Apr 9, 2018

Is this still a WIP? Let us know when this and related pr's are ready for review.

@wjwwood wjwwood closed this Apr 9, 2018
@wjwwood wjwwood reopened this Apr 9, 2018
@wjwwood
Copy link
Copy Markdown
Member

wjwwood commented Apr 9, 2018

Sorry for closing, misclick on my part.

@wjwwood wjwwood added the in progress Actively being worked on (Kanban column) label Apr 9, 2018
@Nickolaim Nickolaim changed the title WIP: count publishers and subscribers Count publishers and subscribers Apr 9, 2018
@Nickolaim Nickolaim mentioned this pull request Apr 13, 2018
Merged
@dirk-thomas dirk-thomas requested review from sloretz and wjwwood April 30, 2018 16:35
@dirk-thomas dirk-thomas added enhancement New feature or request in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Apr 30, 2018
sloretz
sloretz reviewed May 9, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@sloretz sloretz left a comment

Choose a reason for hiding this comment

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

Looks great! Just a couple nitpick comments

rclpy/test/test_node.py Outdated

def test_node_logger(self):
node_logger = self.node.get_logger()
self.assertEqual(node_logger.name, 'my_ns.my_node')
Copy link
Copy Markdown
Contributor

@sloretz sloretz May 9, 2018

Choose a reason for hiding this comment

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

Mind updating this line to use TEST_NODE and TEST_NAMESPACE too?

Copy link
Copy Markdown
Contributor

@sloretz sloretz May 9, 2018

Choose a reason for hiding this comment

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

ca19854

rclpy/rclpy/node.py
validate_topic_name(fq_topic_name)
return func(self.handle, fq_topic_name)

def count_publishers(self, topic_name):
Copy link
Copy Markdown
Contributor

@sloretz sloretz May 9, 2018

Choose a reason for hiding this comment

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

Mind adding a doc string to this and count_subscribers? I realize most functions here don't have one, but adding them to new functions is an improvement.

Copy link
Copy Markdown
Contributor

@sloretz sloretz May 9, 2018

Choose a reason for hiding this comment

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

66bdf9c

@sloretz
Copy link
Copy Markdown
Contributor

sloretz commented May 9, 2018

CI

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@sloretz
Copy link
Copy Markdown
Contributor

sloretz commented May 10, 2018

@Nickolaim Would you mind rebasing your branch with master? CI looks ok except for test failures caused by not having #182.

@Nickolaim
Copy link
Copy Markdown
Contributor Author

Nickolaim commented May 10, 2018

Sure, will do later today. I am relatively new to github; for the request I plan to add remote upstream that points to ros2/rclpy and merge the changes from it. Does it sound good?

@Karsten1987
Copy link
Copy Markdown
Contributor

Karsten1987 commented May 10, 2018

I assume here your remote is called upstream, points to ros2/rclpy and your current branch is pubsub_count3. Then the way to rebase is as follows.

# making sure you update all remotes
git fetch --all 
# rebase your changes on top of current master
git rebase upstream/master

@sloretz
Copy link
Copy Markdown
Contributor

sloretz commented May 10, 2018 •
edited
Loading

@Nickolaim adding on to @Karsten1987's comment, assuming your fork is called origin make sure to

git checkout pubsub_count3
git pull origin pubsub_count3

then after rebasing

git push --force-with-lease origin pubsub_count3

@Nickolaim
Copy link
Copy Markdown
Contributor Author

Nickolaim commented May 11, 2018

I rebased this PR with the master.

@sloretz
Copy link
Copy Markdown
Contributor

sloretz commented May 11, 2018

CI

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@Nickolaim
Copy link
Copy Markdown
Contributor Author

Nickolaim commented May 11, 2018

Looks like all the builds have passed.

sloretz
sloretz approved these changes May 11, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@sloretz sloretz left a comment

Choose a reason for hiding this comment

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

Thanks @Nickolaim !

@sloretz sloretz merged commit 32678d0 into ros2:master May 11, 2018
@sloretz sloretz removed the in review Waiting for review (Kanban column) label May 11, 2018
YuanYuYuan pushed a commit to YuanYuYuan/rclpy that referenced this pull request Nov 12, 2025
@Carter12s
Merge pull request ros2#183 from Carter12s/publisher-latching
65f4c67 
Add latching behavior to native publishers
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sloretz sloretz sloretz approved these changes

@wjwwood wjwwood Awaiting requested review from wjwwood

Assignees

No one assigned

Labels

enhancement New feature or request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@Nickolaim @wjwwood @sloretz @Karsten1987 @dirk-thomas