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

inline namespace constants #202

Merged
merged 1 commit into from
Oct 30, 2018
Merged

inline namespace constants #202

merged 1 commit into from
Oct 30, 2018

Conversation

nholthaus
Copy link
Owner

in the spirit of our other namespace inlining, I'm inlining the constants namespace as well

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 4cbe8b4 on inline-constants-namespace into dca7db2 on v3.x.

@JohelEGP
Copy link
Contributor

I hadn't suggested it due to constants like h, which clash with their equivalently named conversion_factor, due to my desire now expressed in #196, and future constants possibly having the same problem. Not that I suggest using the conversion_factors with the abbreviated name.

@nholthaus
Copy link
Owner Author

nholthaus commented Oct 26, 2018 via email

@JohelEGP
Copy link
Contributor

What do you mean? Right now, we have m, meter and meters, which are conversion_factors, and the unit meter_t. Following the units' renaming, there would be m and meter, and meters, respectively. There's also the issue of #159 (comment).

@nholthaus
Copy link
Owner Author

@JohelEGP once I clear the PR queue, I'm thinking of ding the unit rename, e,g. removing the _t. To do that, I'll first have to change the conversion factor names. I'm thinking unit_name_singular + _conversion. What do you think?

Once that's done, the abbreviations will be free again and we can do the h(1) defines, as well as define the SI unit of each dimension in core.h

@nholthaus nholthaus merged commit a78e18d into v3.x Oct 30, 2018
@JohelEGP
Copy link
Contributor

That sounds good. I'll tell you if I can come up with anything better.

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

Successfully merging this pull request may close these issues.

3 participants