Add get_by_comment method to NumberedObjectCollection - #1006
digvijay-y wants to merge 4 commits into
Conversation
…ng tests - Implemented a method to yield objects matching a comment string or regex pattern. - Added unit tests to validate functionality and error handling for the new method.
digvijay-y
changed the title
Signed-off-by: DIGVIJAY <144053736+digvijay-y@users.noreply.github.com>
|
The test failure isn't your fault, but could you fix it while you have a PR open? The problem is that setuptools-scm changed their website structure. Could you change: https://setuptools-scm.readthedocs.io/en/latest/ to https://setuptools-scm.readthedocs.io/latest/ in the documentation? |
MicahGale
left a comment
MicahGale
left a comment
There was a problem hiding this comment.
This is looking pretty good. Only that needs changed for sure is the changelog. Otherwise let's discuss the other concerns I had.
| if not isinstance(searcher.pattern, str): | ||
| raise TypeError( | ||
| f"searcher must be a str, or a pattern compiled from a str. {searcher} given." | ||
| ) |
There was a problem hiding this comment.
Is it possible to make a pattern from not a string? I'm not familiar with when this branch would apply. Depending on the prevalence of re.compile(1) I would suggest simplifying this this logic, and just doing a basic type enforcement, and not worrying too much about how the pattern was compiled, but let's discuss if this case is common first.
There was a problem hiding this comment.
Technically it can by bytes (e.g. b"\[a-Z]+")
There was a problem hiding this comment.
I'm ok with allowing people to search with byte-strings. It might just find nothing.
| cp_simple_problem.materials.check_number(-1) | ||
|
|
||
| def test_get_by_comment(self): | ||
| problem = montepy.read_input(os.path.join("tests", "inputs", "pin_cell.imcnp")) |
There was a problem hiding this comment.
Is there a reason to use this test case over the test.imcnp given by the fixture read_simple_problem? I haven't looked at how many comments there are recently.
MicahGale
left a comment
MicahGale
left a comment
There was a problem hiding this comment.
Looking good overall. Patch coverage looks to be 100%. Just need to resolve that broken link discussion from earlier, and the one question I had about the test case.
This branch has not been deployed
3 participants
Pull Request Checklist for MontePy
Description
Fixes #982
General Checklist
blackversion 25 or 26.LLM Disclosure
Are you?
Were any large language models (LLM or "AI") used in to generate any of this code?
Documentation Checklist
.. versionchanged::or.. versionadded::directives.Infrastructure Changes
Significant features or Behavior changes
First-Time Contributor Checklist
pyproject.tomlif you wish to do so.Additional Notes for Reviewers
Ensure that:
📚 Documentation preview 📚: https://montepy--1006.org.readthedocs.build/en/1006/