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

Deprecate utilities in favor of ign-utils #171

Closed
chapulina opened this issue May 14, 2021 · 2 comments · Fixed by #233
Closed

Deprecate utilities in favor of ign-utils #171

chapulina opened this issue May 14, 2021 · 2 comments · Fixed by #233
Labels
enhancement New feature or request help wanted Extra attention is needed proposal

Comments

@chapulina
Copy link
Contributor

The ign-utils package has been created to hold general purpose utilities that have minimal dependencies and are useful for multiple libraries. We've already moved the SuppressWarning functionality there.

I think we should also move the ExtraTestMacros and to ign-utils and deprecate the utilities component of this package. I can see an argument for keeping that here close to other test configurations though. My main concern is that this repository is not well suited to hold C++ code. We have created a new namespace, utilities, which doesn't match the rest of this library. We aren't generating test coverage for it, etc. I think it's a cleaner separation of concerns if we leave C++ code out of ign-cmake.

Desired behavior

On ign-cmake3, using ignition::utilities generates deprecation warnings. On ign-cmake4, the component is removed.

I suggest this with the full understanding that ign-cmake4 may only be released in 2078 🙃

My short-term suggestion is to migrate these utilities to ign-utils and stop using the ones from ign-cmake, even though they're not officially deprecated on ign-cmake2.

Alternatives considered

We could just keep ExtraTestMacros here... That would certainly involve less effort. My main concern is that we forget the separation of concerns and start adding more header-only functionality here, when it should go to ign-utils.

Implementation suggestion

  1. Add ExtraTestMacros to ign-utils
  2. Migrate all uses of utilities to use ign-utils
  3. Remove all dependencies on the utilities component from Fortress
  4. Initiate the tick-tock of utilities suggested above.

Additional context

See #18 for why the utilities were added in the first place.

@chapulina chapulina added enhancement New feature or request help wanted Extra attention is needed proposal labels May 14, 2021
@mjcarroll
Copy link
Contributor

This copies ExtraTestMacros into ign-utils: gazebosim/gz-utils#37

@chapulina
Copy link
Contributor Author

Fully deprecated as of #233

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed proposal
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants