Skip to content

Add hacky fix for buggy Options().getAll() - #62

Merged
connorjward merged 3 commits into
mainfrom
connorjward/options-fix
Oct 19, 2026
Merged

Add hacky fix for buggy Options().getAll()#62
connorjward merged 3 commits into
mainfrom
connorjward/options-fix

Conversation

@connorjward

@connorjward connorjward commented Oct 16, 2026

Copy link
Copy Markdown
Collaborator

No description provided.

JHopeCollins
JHopeCollins reviewed Oct 19, 2026
View reviewed changes
Comment thread tests/test_options.py
JHopeCollins
JHopeCollins reviewed Oct 19, 2026
View reviewed changes
Comment thread petsctools/options.py Outdated
JHopeCollins
JHopeCollins reviewed Oct 19, 2026
View reviewed changes
Comment thread tests/test_options.py
JHopeCollins
JHopeCollins reviewed Oct 19, 2026
View reviewed changes
Comment thread tests/test_options.py Outdated
JHopeCollins
JHopeCollins reviewed Oct 19, 2026
View reviewed changes
Comment thread tests/test_options.py Outdated
@connorjward

connorjward commented Oct 19, 2026

Copy link
Copy Markdown
Collaborator Author

@JHopeCollins this all sucks but this change (just one line) is needed to make petsctools.Options().getAll() not raise KeyErrors. I added the tests because the behaviour is non-obvious and it makes it explicit.

It should definitely be fixed upstream, but this PR gets things working for me right now.

@JHopeCollins

JHopeCollins commented Oct 19, 2026

Copy link
Copy Markdown
Member

@JHopeCollins this all sucks but this change (just one line) is needed to make petsctools.Options().getAll() not raise KeyErrors. I added the tests because the behaviour is non-obvious and it makes it explicit.

It should definitely be fixed upstream, but this PR gets things working for me right now.

I understand what is going on now. I'm happy to approve the fix but the comments need updating so that it is clear what is going on.

connorjward and others added 2 commits October 19, 2026 14:07
@connorjward @JHopeCollins
Update petsctools/options.py
0105a3e 
Co-authored-by: Josh Hope-Collins <joshua.hope-collins13@imperial.ac.uk>
@connorjward
Improve comments and add extra getAll test
4923345
@connorjward

connorjward commented Oct 19, 2026

Copy link
Copy Markdown
Collaborator Author

@JHopeCollins I've improved the comments and also added another test for getAll(). There are a couple unresolved comments I'd be grateful if you could close (assuming you're happy).

JHopeCollins
JHopeCollins approved these changes Oct 19, 2026
View reviewed changes

@JHopeCollins JHopeCollins left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for bearing with all my comments.
Approving this PR but please can you open an issue for the options.clear bug just so we have a record later?

@connorjward
connorjward merged commit 6d55dba into main Oct 19, 2026
2 checks passed
@connorjward connorjward mentioned this pull request Oct 19, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JHopeCollins JHopeCollins JHopeCollins approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@connorjward @JHopeCollins