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

change naming of defines to include namespace #18

Closed
dirk-thomas opened this issue Apr 11, 2014 · 14 comments
Closed

change naming of defines to include namespace #18

dirk-thomas opened this issue Apr 11, 2014 · 14 comments

Comments

@dirk-thomas
Copy link
Member

The currently defined macros to log messages (e.g.

#define logWarn(fmt, ...) console_bridge::log(__FILE__, __LINE__, console_bridge::CONSOLE_BRIDGE_LOG_WARN, fmt, ##__VA_ARGS__)
) 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
Copy link
Contributor

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

@scpeters
Copy link
Contributor

scpeters commented Dec 9, 2015

@dirk-thomas ping

@dirk-thomas
Copy link
Member Author

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

scpeters added a commit to scpeters/console_bridge that referenced this issue Dec 9, 2015
Prepend CONSOLE_BRIDGE_ to macros in console.h
Fixes ros#18.
scpeters added a commit to scpeters/console_bridge that referenced this issue Dec 9, 2015
Add duplicate macros to console.h with
`CONSOLE_BRIDGE_` prepended.
First step in fixing ros#18.
scpeters added a commit to scpeters/console_bridge that referenced this issue Dec 14, 2015
Add duplicate macros to console.h with
`CONSOLE_BRIDGE_` prepended.
First step in fixing ros#18.
@wjwwood
Copy link
Member

wjwwood commented Jan 26, 2016

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

@scpeters
Copy link
Contributor

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
Copy link
Member

wjwwood commented Jan 26, 2016

Sounds good to me.

@mikaelarguedas
Copy link
Member

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

@scpeters
Copy link
Contributor

should we call it a 1.0?

@mikaelarguedas
Copy link
Member

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
Copy link
Member Author

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

@scpeters
Copy link
Contributor

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
Copy link
Member

wjwwood commented Nov 16, 2017

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
Copy link
Contributor

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
Copy link
Contributor

see #48

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

No branches or pull requests

4 participants