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

add MolarEnergy, implement Energy unimplemented method #274

Merged
merged 1 commit into from
Jun 25, 2017

Conversation

nvinuesa
Copy link
Contributor

This is the 5th PR concerning #252 and the first one implementing unimplemented methods and missing units in energy.

@nvinuesa nvinuesa force-pushed the issue-252-5 branch 3 times, most recently from 7550b0f to ab7e211 Compare June 23, 2017 21:06
* @author Nicolas Vinuesa
* @since 1.4
*
* @param value Double
Copy link
Collaborator

Choose a reason for hiding this comment

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

Some description?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I mean, I'm not very familiar with MolarEnergy, perhaps we could add a link to wikipedia?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Its just a unit of energy over mass. The link could be: https://en.wikipedia.org/wiki/Joule_per_mole
Should I add it in the class? I've not seen it elsewhere...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, you're right. I've seen only one case but that is in another level. Nevermind

Copy link
Collaborator

@cquiroz cquiroz left a comment

Choose a reason for hiding this comment

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

Looks good in general, I'll wait for others to comment before approving

@derekmorr
Copy link
Collaborator

LGTM. We'll need to add this to ImplicitDimensions too.

Copy link
Collaborator

@derekmorr derekmorr left a comment

Choose a reason for hiding this comment

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

Add to Implicit Dimensions class.

@derekmorr derekmorr modified the milestone: 1.4 Jun 25, 2017
@nvinuesa
Copy link
Contributor Author

@derekmorr it is already in implicitDimensions ... check the commit

@derekmorr
Copy link
Collaborator

Ah, sorry. Too many commits this morning.

@garyKeorkunian
Copy link
Contributor

I think that misspelling of JoulesToMole should be corrected before this is merged.

@nvinuesa
Copy link
Contributor Author

Of course, i'll get it done right away

@nvinuesa
Copy link
Contributor Author

@garyKeorkunian done!

Copy link
Contributor

@garyKeorkunian garyKeorkunian left a comment

Choose a reason for hiding this comment

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

LGTM

def apply[A](n: A)(implicit num: Numeric[A]) = MolarEnergy(n, this)
}

object JoulesPerMol extends MolarEnergyUnit with PrimaryUnit with SiUnit {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be JoulesPerMole?


def *(that: ChemicalAmount): Energy = Joules(this.toJoulesPerMol * that.toMoles)

def toJoulesPerMol = to(JoulesPerMol)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here ... toJoulesPerMole

@garyKeorkunian garyKeorkunian merged commit f15a5a3 into typelevel:master Jun 25, 2017
@nvinuesa nvinuesa deleted the issue-252-5 branch July 25, 2017 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants