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

Rename density to mass density #130

Merged
merged 1 commit into from
May 5, 2019

Conversation

Aehmlo
Copy link
Contributor

@Aehmlo Aehmlo commented Apr 29, 2019

"Mass density" is the more rigorous and technically correct version (though I believe the SI brochure does essentially say they're equivalent), so I think we should at least expose this as an option.

To retain backwards compatibility, this PR contains pub type Density<U, V> = MassDensity<U, V>. I'd rather not have this from an idealistic standpoint, but from a practical perspective, it makes sense to not break backwards compatibility for this (which I would expect to be one of the more commonly-used quantities). We could also just make the type alias work the other way around (pub type MassDensity<U, V> = Density<U, V>) and thereby obtain a single-line diff (two lines with docs), but I do think that this is the more semantically correct version.

@coveralls
Copy link

coveralls commented Apr 29, 2019

Coverage Status

Coverage remained the same at 97.229% when pulling 8a2940b on Aehmlo:mass-density into 8c5eb43 on iliekturtles:master.

@iliekturtles
Copy link
Owner

I agree that MassDensity is the better name. I can't remember why Density was chosen over MassDensity but I'll do a bit of digging when I get a moment. I think it would also be good to put a #[deprecated] attribute on the alias to the old name in order to get users to switch.

@Aehmlo
Copy link
Contributor Author

Aehmlo commented Apr 30, 2019

Good idea, I’ll add that.

Copy link
Owner

@iliekturtles iliekturtles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once the comments I made in the diff are resolved I'll merge!

  • Update comment on kilogram_per_cubic_meter to say "... unit of mass density."

Introduce type alias for backwards compatibility and mark Density as
deprecated.
@iliekturtles iliekturtles merged commit 8a2940b into iliekturtles:master May 5, 2019
iliekturtles added a commit that referenced this pull request May 5, 2019
Rename density to mass density.
@iliekturtles
Copy link
Owner

Thanks! I manually merged and resolved the conflicts with #129.

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