Skip to content

Use direct assingment for settings#133

Merged
bnmajor merged 1 commit into
mainfrom
fix-simulation-tests
Jul 8, 2026
Merged

Use direct assingment for settings#133
bnmajor merged 1 commit into
mainfrom
fix-simulation-tests

Conversation

@bnmajor

@bnmajor bnmajor commented Jun 17, 2026

Copy link
Copy Markdown
Collaborator

No description provided.

@bnmajor
bnmajor requested a review from marySalvi June 17, 2026 14:28
@@ -99,7 +99,7 @@ def test_run_phold_simulation_success(
self, mock_call_command: mock.Mock, settings: object

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The problem here is the type annotation on settings. It shouldn't be object.

I think it's fine to leave it as unannotated, which shouldn't cause any errors (please let me know if it does).

The actual type is SettingsWrapper, coming from from pytest_django.fixtures import SettingsWrapper.

self, mock_call_command: mock.Mock, settings: object
) -> None:
"""Test successful PHOLD simulation execution."""
setattr(settings, "PHOLD_BINARY_PATH", "/path/to/phold") # noqa: B010

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

In general, if we're writing worse, more-confusing code just to get around some sort of QA tool, then I'm fine to declare that the tool is probably wrong. I'd always appreciate hearing immediate feedback about these sort of cases, so we can investigate and make our tools more ergonomic.

@bnmajor
bnmajor force-pushed the fix-simulation-tests branch from 5220653 to 22383d8 Compare July 6, 2026 14:12
@bnmajor
bnmajor requested review from marySalvi and removed request for marySalvi July 6, 2026 14:12
@bnmajor
bnmajor merged commit aed2bd2 into main Jul 8, 2026
2 checks passed
@bnmajor
bnmajor deleted the fix-simulation-tests branch July 8, 2026 16:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants