Yaml2ck fixes - Githubissues

Cantera / cantera

Chemical kinetics, thermodynamics, and transport tool suite

https://cantera.org

Other

624 stars 350 forks source link

Yaml2ck fixes #1733

Closed corykinney closed 4 months ago

corykinney commented 4 months ago

Changes proposed in this pull request

This pull request would fix a couple of issues with yaml2ck:

Multi-line species thermo notes now have the comment marker ! at the beginning of each line
Writing a phase with no reactions will no longer crash, as the REACTIONS section is omitted

If applicable, fill in the issue number this pull request is fixing

Closes #1633 and closes #1679

Checklist

[x] The pull request includes a clear description of this code change
[x] Commit messages have short titles and reference relevant issues
[x] Build passes (scons build & scons test) and unit tests address code coverage
[x] Style & formatting of contributed code follows contributing guidelines
[ ] The pull request is ready for review

corykinney commented 4 months ago

Should I implement test cases for these, or are these changes simple and straightforward enough as it is?

codecov[bot] commented 4 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 75.72%. Comparing base (026c5c7) to head (8db13b0). Report is 98 commits behind head on main.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #1733 +/- ## ========================================== + Coverage 75.69% 75.72% +0.03% ========================================== Files 443 445 +2 Lines 60971 61509 +538 Branches 9552 9652 +100 ========================================== + Hits 46154 46580 +426 - Misses 11786 11871 +85 - Partials 3031 3058 +27 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

speth commented 4 months ago

Should I implement test cases for these, or are these changes simple and straightforward enough as it is?

They're simple changes, but I think some test cases would be useful to prevent any inadvertent regression in behavior.

corykinney commented 4 months ago

@speth Okay, I think the test cases added should be sufficient (and I've confirmed they fail apart from the changes implemented, so they should address what they were intended to).

EDIT: For the phase with no reactions test, it only checks the Chemkin file was written, should that be enough?