Fixing Units.pm - value of electron volt#678
Conversation
Correct exponential for factor of eV fixes openwebwork#677 Adds definitions for kev, MeV and GeV Adds unit tests for Units.pm Adds cpanfile for external module dependencies Updates t/README with description of integration tests, unit tests, instructions for how to run these tests and how to use the cpanfile
|
Please either re-target this to the develop branch or the PG-2.17 release candidate branch. We will not be adding hotfixes to the 2.16 release anymore. Edit: Make that the PG-2.17 branch. |
|
In the Edit, did you mean that I should target the PG-2.17 branch, not the release candidate or target the PG-2.17 branch, not the develop branch? |
|
Yes. You should target the PG-2.17 branch. That is the release candidate. The main branch is the current release. The develop branch is for future development that will not go into the upcoming release. The release candidate PG-2.17 branch will be merged into both main and develop when the actual release occurs at the end of July. |
|
Overall this looks good, but needs to be targeted on the other branch. |
1 similar comment
|
Overall this looks good, but needs to be targeted on the other branch. |
drgrice1
left a comment
drgrice1
left a comment
There was a problem hiding this comment.
I think that this will conflict with the unit tests that have already been added to the develop and release candidate branches. It certainly will conflict with the README.md file.
I am not sure that I agree with the addition of the perl Test2::Suite module either. I think it would be best to use the Test::More module already in use in the existing tests. We don't need two different test suites. There needs to be consistency in the way that the test files work as well. The tests you are adding don't work the same as the existing tests.
I don't really think that the addition of the cpanfile is necessary.
|
I took the opportunity to add more tests and build up the test framework. A new PR targeting PG-2.17 is in the pipeline. Using a cpanfile is standard in software development these days for automating building and deployment stages. It allows you to "pin" dependencies to specific versions in the case of known issues and it doesn't interfere with people whose workflows don't use it. |
3 participants
This PR corrects the exponential for factor of eV in lib/Units.pm, which was out by a factor 10^10.
fixes #677
I've taken the opportunity to add definitions for
keV,MeVandGeV. These units are used in particle physics and for gamma ray energies. The indentation in that section was tidied.I've also added unit test files for Units.pm and updated the t/README with instructions and rationale. I've found that the Test2 suite is a good framework for writing tests. To make installing module dependencies easier, I've included a cpanfile and instructions for different use cases.