Skip to content

Add None to choices of optional enum in metadata.py#118

Merged
rjoussen merged 1 commit into4C-multiphysics:mainfrom
rjoussen:fix-enum-choices
Mar 5, 2026
Merged

Add None to choices of optional enum in metadata.py#118
rjoussen merged 1 commit into4C-multiphysics:mainfrom
rjoussen:fix-enum-choices

Conversation

@rjoussen
Copy link
Copy Markdown
Contributor

@rjoussen rjoussen commented Mar 5, 2026

This should fix #117.

The latest metadata change in 4C (#1725) fixed that optional enums need None (null in yaml) as a valid choice additionally. #1688 used this feature for the first time, that's why it failed now.

This PR basically copies the changes made in 4C #1725

@davidrudlstorfer

@davidrudlstorfer davidrudlstorfer requested a review from gilrrei March 5, 2026 09:44
@davidrudlstorfer
Copy link
Copy Markdown
Collaborator

@gilrrei maybe you could have a quick look :)

@rjoussen
Copy link
Copy Markdown
Contributor Author

rjoussen commented Mar 5, 2026

@davidrudlstorfer What is the usual workflow for issues like this? Are the fourcipp metadata.py and 4C metadata.py expected to be the same? (or at least almost the same, in regard to functionality) Because it seems like they are, except for very small differences due to some recent changes. Even the Enum(InputSpec) class still has some differences that are not crucial to fixing #117, so I did not port them.

@gilrrei
Copy link
Copy Markdown
Collaborator

gilrrei commented Mar 5, 2026

@rjoussen Thanks for the fix :)

What is the usual workflow for issues like this? Are the fourcipp metadata.py and 4C metadata.py expected to be the same?

tl;dr No current workflow, fix as we go

Optimally, they should be identical. There were some ideas in the past to keep them synced; however, it's not so easy without creating cyclic dependencies. In combination with a lack of consensus on what is/isn't part of 4C and, therefore, a lack of a combined testing infrastructure, fourcipp is in this limbo where mismatches are only caught during nightly checks or manually in 4C PRs by fourcipp enthusiasts like @davidrudlstorfer :) Afterwards, they have to be fixed manually in fourcipp.

@rjoussen
Copy link
Copy Markdown
Contributor Author

rjoussen commented Mar 5, 2026

@gilrrei Thanks for the explanation. It could be worth amending the copilot instructions for reviews by something like: "Changes in metadata.py need to be ported to fourcipp"

@rjoussen rjoussen merged commit f68f299 into 4C-multiphysics:main Mar 5, 2026
8 checks passed
@davidrudlstorfer
Copy link
Copy Markdown
Collaborator

@rjoussen could you actually open a PR in 4C for your proposal regarding your addition to the copilot review instructions?:)

@rjoussen
Copy link
Copy Markdown
Contributor Author

rjoussen commented Mar 5, 2026

@davidrudlstorfer done.

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.

Fix failing pipeline and publish

3 participants