ianmackenzie / elm-units

Simple, safe and convenient unit types and conversions for Elm
https://package.elm-lang.org/packages/ianmackenzie/elm-units/latest/
BSD 3-Clause "New" or "Revised" License
84 stars 14 forks source link

Add cubicCentimeters to Volume module #54

Closed g-belmonte closed 3 years ago

g-belmonte commented 3 years ago

Hey there, @ianmackenzie !

I opened this MR implementing the cubic centimeters as an alias of milliliters, as I understood from https://github.com/ianmackenzie/elm-units/issues/52

Let me know your thoughts :smile:

ianmackenzie commented 3 years ago

Looks good, and thanks for adding tests as well! Only a couple very minor comments:

ianmackenzie commented 3 years ago

Oh, and if you could add yourself to the AUTHORS file that would be great 🙂

g-belmonte commented 3 years ago

small doubt: at the module's exposing, should I leave the cubicCentimeters with the other cubic things, or after the milliliters, as in the body of the code?

g-belmonte commented 3 years ago

another doubt: should I add the cubicCentimeter to the list of unitary quantities at the end of the file?

ianmackenzie commented 3 years ago

Oh actually I think the issue is that cubicCentimeters and inCubicCentimeters need to be added to the @docs line after milliliters/inMilliliters; as long as you're using elm-format then they'll automatically be moved to the corresponding place in the module exposing clause. (You do have to make sure they've been added to the exposing clause somewhere first, though.)

Good catch on adding cubicCentimeter! Yes, please do add that (to the code, @docs line and exposing clause - again, if you put it just after milliliter in the @docs line then elm-format should move it to the right place in the exposing clause.)

g-belmonte commented 3 years ago

Adjustments done! :smile:

Checklist:

I hope I didn't miss anything :grin:

ianmackenzie commented 3 years ago

Looks great, thanks!