-
Notifications
You must be signed in to change notification settings - Fork 11
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 failing test case for issue #549 (empty Importance object print) #681
base: develop
Are you sure you want to change the base?
Add failing test case for issue #549 (empty Importance object print) #681
Conversation
…ject not connected to _problem
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution @kordusas!
This is a good first pass but some changes are needed. I left comments on specific changes.
- Update the PR description to give a summary of what the problem that was solved is, and how it was solved. Also link to the specific issues. See Allow any Real type for floating point numbers and any Integral type for integer numbers during type enforcement #680 for an example.
- Do you want to add yourself as an author to
AUTHORS
and/orpyproject.toml
? - Make sure to update the changelog with this bug fix.
- Would you like to added as a contributor to this repo on GH?
if self._problem: | ||
# Use the full formatting (which relies on _problem.cells, etc.) | ||
return "".join(self.format_for_mcnp_input(DEFAULT_VERSION)) | ||
else: | ||
# Fall back to a default simple representation | ||
default = self._format_default() | ||
if default: | ||
return " ".join(default) | ||
return "Importance object is empty" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you should think beyond the previous implementation, and really think about what information is actually useful to the user here? Focus more on what information is important than reformatting the syntax tree.
Maybe something like
>>> str(imp)
Importance: {n: 1.0, p: 2.0...}
@tjlaboss has stronger opinions on these matters. Here's our current guidance: https://www.montepy.org/en/stable/developing.html#how-to-str-vs-repr
def test_importance_init_empty(self): | ||
importance = Importance() | ||
assert str(importance) == "Importance object is empty" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good start. I would like to see this test multiple scenarios though, and this test suite needs to be updated to be canonically pytest.
I'm thinking:
- Remove all use of
TestCase
, see this example: https://github.com/idaholab/MontePy/blob/alpha-test-dev/tests/test_material.py - Make a
pytest.fixture
that sets up and returns the two cells from the previous test - Test
str
andrepr
for:- parsed cells from the fixture
- an empty importance,
- an importance/cell made from scratch
If this is too much infrastructure changes, let me know. I can do a PR onto your branch for some of it.
…turing list added type hints to the function signature
Pull Request Checklist for MontePy
Description
added pytest to teset the empty importance object printing
Fixes # (issue number)
General Checklist
black
version 25.Documentation Checklist
I have documented all added classes and methods.For infrastructure updates, I have updated the developer's guide.For significant new features, I have added a section to the getting started guide.First-Time Contributor Checklist
pyproject.toml
if you wish to do so.Additional Notes for Reviewers
Ensure that: