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.

Sign up for GitHub

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

Jump to bottom

Replace deprecated yaml.safe_load / simplify paths #1049

Merged
merged 10 commits into from
Jun 11, 2021
Merged

Replace deprecated yaml.safe_load / simplify paths #1049

merged 10 commits into from
Jun 11, 2021

Conversation

ischoegl
Copy link
Member

@ischoegl ischoegl commented May 31, 2021 •
edited
Loading

Changes proposed in this pull request

  • Replace deprecated yaml.safe_load
  • Implement dedicated loader in test/utilities.py
  • Allow for loading of Solution from Path
  • Transition some unit tests to pathlib

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

Fixes #1048

If applicable, provide an example illustrating new features this pull request is introducing

Checklist

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

@ischoegl ischoegl requested a review from a team May 31, 2021 21:59
@ischoegl ischoegl added the CI label May 31, 2021
@ischoegl
Copy link
Member Author

ischoegl commented Jun 1, 2021

Fixed a couple of " that used to be '

@ischoegl ischoegl force-pushed the fix-safe-load branch 2 times, most recently from ac818e4 to 7f58c26 Compare June 1, 2021 23:34
@ischoegl ischoegl changed the title [CI] Replace deprecated yaml.safe_load Replace deprecated yaml.safe_load / simplify paths Jun 1, 2021
@ischoegl ischoegl force-pushed the fix-safe-load branch 3 times, most recently from f5a31e9 to 59752c6 Compare June 2, 2021 00:40
@ischoegl
Copy link
Member Author

ischoegl commented Jun 2, 2021

Ended up removing references to os.path in favor of pathlib for the Python test suite.

bryanwweber
bryanwweber requested changes Jun 2, 2021
View reviewed changes
Copy link
Member

@bryanwweber bryanwweber left a comment

Choose a reason for hiding this comment

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

Thanks for taking this on @ischoegl, I've been wanting to switch more stuff to pathlib for a while!

interfaces/cython/cantera/base.pyx Outdated Show resolved Hide resolved
interfaces/cython/cantera/test/test_composite.py Show resolved Hide resolved
interfaces/cython/cantera/test/test_composite.py Outdated Show resolved Hide resolved
interfaces/cython/cantera/test/test_composite.py Show resolved Hide resolved
interfaces/cython/cantera/test/test_kinetics.py Outdated Show resolved Hide resolved
interfaces/cython/cantera/utils.pyx Outdated Show resolved Hide resolved
interfaces/cython/cantera/test/test_convert.py Outdated Show resolved Hide resolved
interfaces/cython/cantera/test/test_convert.py Outdated Show resolved Hide resolved
interfaces/cython/cantera/test/test_thermo.py Outdated Show resolved Hide resolved
interfaces/cython/cantera/test/utilities.py Outdated Show resolved Hide resolved
@ischoegl ischoegl requested a review from bryanwweber June 2, 2021 04:04
@ischoegl
Copy link
Member Author

ischoegl commented Jun 2, 2021

@bryanwweber ... replacing the remaining path strings created some more churn (as expected), but now everything is consistent.

@ischoegl
[Tests] Remove unused imports in Python tests
94ddaa4 
Also:
* introduce module constant utilities.TEST_DATA_PATH
* update tests/__init__.py
bryanwweber
bryanwweber requested changes Jun 2, 2021
View reviewed changes
Copy link
Member

@bryanwweber bryanwweber left a comment

Choose a reason for hiding this comment

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

Just a few minor changes from the updates. Thanks again @ischoegl!

interfaces/cython/cantera/test/test_convert.py Show resolved Hide resolved
interfaces/cython/cantera/test/test_convert.py Outdated Show resolved Hide resolved
interfaces/cython/cantera/test/test_convert.py Outdated Show resolved Hide resolved
interfaces/cython/cantera/test/test_convert.py Outdated Show resolved Hide resolved
interfaces/cython/cantera/test/test_convert.py Outdated Show resolved Hide resolved
interfaces/cython/cantera/test/utilities.py Outdated Show resolved Hide resolved
@ischoegl ischoegl requested a review from bryanwweber June 2, 2021 13:26
@ischoegl
Copy link
Member Author

@bryanwweber ... I believe everything is addressed. Unless there's anything else, this may be ready for a merge.

@bryanwweber bryanwweber merged commit e60f513 into Cantera:main Jun 11, 2021
@ischoegl
Copy link
Member Author

Thanks @bryanwweber!

@ischoegl ischoegl deleted the fix-safe-load branch June 11, 2021 14:47
@ischoegl ischoegl mentioned this pull request Jun 11, 2021
Merged
8 tasks
@DavidAkinpelu DavidAkinpelu mentioned this pull request Jun 14, 2021
Closed
@ischoegl ischoegl mentioned this pull request Jun 14, 2021
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bryanwweber bryanwweber bryanwweber approved these changes

Assignees
No one assigned
Labels
CI
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

yaml.safe_load broke in CI
2 participants
@ischoegl @bryanwweber