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

Use state instead of Settings #424

Merged
merged 5 commits into from
Nov 30, 2021
Merged

Use state instead of Settings #424

merged 5 commits into from
Nov 30, 2021

Conversation

liamhuber
Copy link
Member

@liamhuber liamhuber commented Nov 15, 2021

A surprising amount of the time that just meant getting rid of the Settings import, as it was unused. I also snuck in a couple other unused import removals and some encoding info additions here and there, depending on the repo.

A surprising amount of the time that just meant getting rid of the Settings import, as it was unused. I also snuck in a couple other unused import removals and some encoding info additions here and there, depending on the repo.
@niklassiemer
Copy link
Member

You are a bit early, since the state PR is not yet released to conda 😃

@liamhuber liamhuber marked this pull request as draft November 15, 2021 12:23
@liamhuber
Copy link
Member Author

You are a bit early, since the state PR is not yet released to conda 😃

😆 yep, we're looking at it simultaneously

@niklassiemer niklassiemer marked this pull request as ready for review November 24, 2021 14:46
@niklassiemer niklassiemer marked this pull request as draft November 24, 2021 14:47
@coveralls
Copy link

Pull Request Test Coverage Report for Build 1516971076

  • 48 of 63 (76.19%) changed or added relevant lines in 25 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage decreased (-0.04%) to 69.823%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pyiron_atomistics/atomistics/job/interactive.py 1 2 50.0%
pyiron_atomistics/atomistics/structure/phonopy.py 1 2 50.0%
pyiron_atomistics/lammps/base.py 2 3 66.67%
pyiron_atomistics/sphinx/base.py 3 4 75.0%
pyiron_atomistics/vasp/potential.py 3 4 75.0%
pyiron_atomistics/vasp/vasprun.py 1 2 50.0%
pyiron_atomistics/sphinx/volumetric_data.py 1 3 33.33%
pyiron_atomistics/vasp/volumetric_data.py 2 5 40.0%
pyiron_atomistics/vasp/base.py 6 10 60.0%
Files with Coverage Reduction New Missed Lines %
pyiron_atomistics/atomistics/job/sqs.py 1 46.9%
Totals Coverage Status
Change from base Build 1516931863: -0.04%
Covered Lines: 11414
Relevant Lines: 16347

💛 - Coveralls

@liamhuber liamhuber marked this pull request as ready for review November 29, 2021 18:54
@liamhuber liamhuber added the integration Start the notebook integration tests for this PR label Nov 29, 2021
Copy link
Member

@samwaseda samwaseda left a comment

Choose a reason for hiding this comment

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

I'm not particularly happy with the fact that there's no test, but other than that it looks good to me. :)

@liamhuber
Copy link
Member Author

I'm not particularly happy with the fact that there's no test, but other than that it looks good to me. :)

We'll, the basic state stuff is tested over in base. It's true though that I didn't add any special tests, and Marvin did indeed track down a state related (but maybe not state or caused?) issue. I don't know how to conveniently link issues on my phone in the app though, so you'll just have to go look for it 🤣

@liamhuber liamhuber merged commit 63928b1 into master Nov 30, 2021
@delete-merged-branch delete-merged-branch bot deleted the use_state branch November 30, 2021 18:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integration Start the notebook integration tests for this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants