Skip to content

Fix options callbacks#20

Merged
bnmajor merged 2 commits intomasterfrom
callback-fix
Mar 13, 2025
Merged

Fix options callbacks#20
bnmajor merged 2 commits intomasterfrom
callback-fix

Conversation

@bnmajor
Copy link
Copy Markdown
Collaborator

@bnmajor bnmajor commented Feb 25, 2025

In order to support arguments in any order, we now check for algorithm and parallel flag compatibility with the selected environment on algorithm, env_name, and parallel options. This ensures that no matter what defaults are used or what order any argument is passed, checks are always made and any necessary warnings are displayed and errors are raised.

In order to support arguments in any order, check for algorithm and parallel
flag compatability with selected environment on the algorithm, env_name, and
parallel options. This ensures that no matter what defaults are used or what
order any argument is passed in all checks are made and any warnings are
displayed and errors are raised.
Copy link
Copy Markdown
Member

@brianhelba brianhelba left a comment

Choose a reason for hiding this comment

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

I didn't realize that validating across arguments would be this awkward with the callback API. I think this code is fine to fix the immediate issue, but I'm now realizing that we'll probably want to validate the same parameter combination when creating models via other sources (like a web UI).

The right way to handle this should probably be a model-level validation, to keep the logic in a single place: https://docs.djangoproject.com/en/5.1/ref/models/instances/#validating-objects

So, I think we can merge this to handle the immediate bugs, but then move all the logic to a new clean method on the relevant models. To actually get that code (and other internal auto-generated validators) to run, we'll need to shift from creating models via Model.objects.create to instantiating models, then calling .full_clean() on them, then calling .save() (see the aforementioned docs on this).

@bnmajor bnmajor merged commit b192c81 into master Mar 13, 2025
@bnmajor bnmajor deleted the callback-fix branch March 13, 2025 13:18
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.

2 participants