search

ros / console_bridge

A ROS-independent package for logging that seamlessly pipes into rosconsole/rosout for ROS-dependent packages.
BSD 3-Clause "New" or "Revised" License
22 stars 62 forks source link

change naming of defines to include namespace #18

Closed dirk-thomas closed 6 years ago

dirk-thomas commented 10 years ago

The currently defined macros to log messages (e.g. https://github.com/ros/console_bridge/blob/7aebc3367b4945d9963457549026ea137b047b7b/include/console_bridge/console.h#L68) are much to generic and collide with existing code. The should be prefixed with the package name to make them unique.

Even if this is a significant API change this should be fixed. Likely bumping more then just the patch version number.

scpeters commented 9 years ago

So, for example, you would like the logWarn macro to change to something like CONSOLE_BRIDGE_logWarn?

scpeters commented 8 years ago

@dirk-thomas ping

dirk-thomas commented 8 years ago

Yes, prefixing all functions with console_bridge_ and macros with CONSOLE_BRIDGE_ would namespace those to prevent collisions.

wjwwood commented 8 years ago

What remains to be done here? It looks like @scpeters has at least added the new macros as an alternative?

scpeters commented 8 years ago

Yeah, the new macros have been added, and the old ones are deprecated but still present. I was waiting to close this until the next version is released with the old macros redacted.

wjwwood commented 8 years ago

Sounds good to me.

mikaelarguedas commented 6 years ago

@scpeters friendly :bell: Is there still a plan to release a new version with the deprecated macros removed?

scpeters commented 6 years ago

should we call it a 1.0?

mikaelarguedas commented 6 years ago

Hmmm that's a good question, given the ros namespacing constraints I usually do only minor and patch releases even when breaking API. As this package is submitted upstream and not tied to a given distribution I think it would make sense to bump the major. Maybe @tfoote or @dirk-thomas can give feedback on this as well

dirk-thomas commented 6 years ago

Following semver it should be at least 0.4 since it breaks API.

scpeters commented 6 years ago

It depends on your interpretation of semantic versioning. We are extra strict in gazebo to bump major version if API or ABI changes.

It's worth noting that the config version file uses COMPATIBILITY SameMajorVersion.

But I'll defer to you.

wjwwood commented 6 years ago

Also:

Major version zero (0.y.z) is for initial development. Anything may change at any time. The public API should not be considered stable.

-- http://semver.org/#spec-item-4

scpeters commented 6 years ago

That's a fair point. I take it that most are in favor of 0.4? If so I'll make a PR that bumps the version and removes the deprecated macros.

scpeters commented 6 years ago

see #48