search

ros-industrial / ros_canopen

CANopen driver framework for ROS (http://wiki.ros.org/ros_canopen)
GNU Lesser General Public License v3.0
336 stars 271 forks source link

Added configurable queue sizes #342

Closed reidchristopher closed 5 years ago

reidchristopher commented 5 years ago

Allowed the inclusion of private parameters for sent and received message queue sizes closes #253

mathias-luedtke commented 5 years ago

@JeremyZoss: This was not meant to be merged like this. My commit was a fixup, which was meant to be squashed. Only maintainers are entitled to merge.

JeremyZoss commented 5 years ago

My apologies. I was unaware of that restriction. Would you like me to force-push an undo or add a reversion commit? I assumed that an "approved" PR meant that it was ready for merging. Please advise on how you would like me to proceed, or if you plan to fix my error yourself.

mathias-luedtke commented 5 years ago

It cannot be fixed anymore. I think it is an established policy that admins do not merge PRs in repositories (unless they are the maintainers).

I approved the change in e58348a6fb77dde6c2e76ea7a202c7ec0c2de59c, but intended to refactor it towards 686e64f95879f3eff4ac4b0bea130d5d32ef2bc6. I did not flag this, because for the time beeing I am the only maintainer of ros_canopen.

gavanderhoorn commented 5 years ago

I think it is an established policy that admins do not merge PRs in repositories (unless they are the maintainers).

It's not established policy, but we can say it is an unwritten rule / gentlemen's agreement.

I'm sure @JeremyZoss was just trying to be efficient and clean up the project board, so we can avoid what happened last year.

And I must say I also assumed this PR was acceptable as-is, because of:

ipa-mdl approved these changes 19 hours ago

it might be best not to submit accepting reviews unless the PR is actually acceptable.

JeremyZoss commented 5 years ago

I am deeply sorry for causing this issue with your repository. As Gijs said, I was only trying to help clean up the issue board after Travis checks had finished running.

In the future, I will definitely be more conscious to respect the authority maintainers have over their own repositories. If you receive criticism for having a messy commit history, feel free to point the accusatory finger in my direction. =)

mathias-luedtke commented 5 years ago

it might be best not to submit accepting reviews unless the PR is actually acceptable.

The change was alright, but I intended to squash-merge after CI passed. For ros_canopen I don't keep merge commits.

gavanderhoorn commented 5 years ago

it might be best not to submit accepting reviews unless the PR is actually acceptable.

The change was alright, but I intended to squash-merge after CI passed. For ros_canopen I don't keep merge commits.

makes sense.

In that case I always just request a review / do one, then comment that I'll merge / a squash merge should be done.