Codecov Report

Merging #479 (0dfe004) into ign-math6 (eb9183f) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 0dfe004 differs from pull request most recent head 3cddef8. Consider uploading reports for the commit 3cddef8 to get more accurate results

@@            Coverage Diff             @@
##           ign-math6     #479   +/-   ##
==========================================
  Coverage      99.68%   99.68%           
==========================================
  Files             73       73           
  Lines           6913     6913           
==========================================
  Hits            6891     6891           
  Misses            22       22           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

the results here seem inconsistent; sometimes the math:: is removed and sometimes not

the results here seem inconsistent; sometimes the math:: is removed and sometimes not

It's dependent on the appearance of a preceding using namespace math; directive (and a few escape hatches).
I'm not sure if there's a way to programatically determine when it would be good to add in a using directive..

I couldn't get the namespaces removed for class declarations and function arguments and still have it compile as well :o

LGTM, I just have minor comments about being more explicit when the functions could be confused with those in the std:: namespace. It's not a big deal though, because we always use the std namespace explicitly and I believe the compiler (at least the ones we support) would complain if there was an ambiguity.

Speaking of which, I have an explicit escape hatch to not remove the following namespaces :D (mainly because it kept giving me compilation errors)

• std
• google::protobuf
• details

So it's always guaranteed that an explicit use of std:: will be preserved

the results here seem inconsistent; sometimes the math:: is removed and sometimes not

It's dependent on the appearance of a preceding using namespace math; directive (and a few escape hatches). I'm not sure if there's a way to programatically determine when it would be good to add in a using directive..

I couldn't get the namespaces removed for class declarations and function arguments and still have it compile as well :o

I think extra namespaces are sometimes included as a style choice or to make it more obvious where the given method is being called from, so this PR is kind of a big set of style changes. I would have no qualms about this if it only removed ignition:: namespaces and left the math:: namespaces in place.

That said, I'm willing to accept these changes but with hesitation

sometimes included as a style choice or to make it more obvious where the given method is being called from

We haven't been enforcing a specific style when it comes to using namespaces. We've actually not been following the Google style guide too closely, because we use using namespace ignition in the .cc files instead of wrapping the file in namespace ignition {}. I didn't find any recommendations there about repeating a namespace within itself. So I don't think we need to block this PR on this.

We just need to be careful not to call top-level global functions by mistake as it happened in gazebosim/gz-common#414 (comment), because that actually changes the code.

So I don't think we need to block this PR on this.

agreed; I just wanted to express a concern