Skip to content

Fix mutation of data_types causing order-dependent behavior#5037

Open
Adir-html wants to merge 1 commit intoansys:mainfrom
Adir-html:fix-surface-data-types-order
Open

Fix mutation of data_types causing order-dependent behavior#5037
Adir-html wants to merge 1 commit intoansys:mainfrom
Adir-html:fix-surface-data-types-order

Conversation

@Adir-html
Copy link
Copy Markdown

Fix mutation of data_types causing order-dependent behavior

The current implementation mutates the data_types list while iterating over it, which can lead to order-dependent behavior and inconsistent return types.

This PR replaces the mutation logic with a list comprehension that preserves order and avoids in-place modification:

data_types = [
    d_type if isinstance(d_type, SurfaceDataType)
    else SurfaceDataType(d_type)
    for d_type in data_types
]

This ensures consistent behavior regardless of input ordering.

Tested using the reproduction steps from the issue.

Copilot AI review requested due to automatic review settings April 1, 2026 14:40
@Adir-html Adir-html requested a review from mayankansys as a code owner April 1, 2026 14:40
@ansys-cla-bot
Copy link
Copy Markdown

ansys-cla-bot bot commented Apr 1, 2026

The following people have not signed the Contributors License Agreement (CLA):

Read the CLA in the link above and sign it by clicking the link below:

You will receive a confirmation as soon as your signature is captured.

@codacy-production
Copy link
Copy Markdown

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

TIP This summary will be updated as you push new changes. Give us feedback

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Fixes order-dependent behavior in FileFieldData._get_surface_data() by eliminating in-place mutation of the data_types iterable during normalization to SurfaceDataType.

Changes:

  • Replace in-loop remove/append mutation of data_types with a new list constructed via comprehension to preserve input order.
  • Ensure data_types entries are normalized to SurfaceDataType before downstream membership checks.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +693 to +696
d_type if isinstance(d_type, SurfaceDataType)
else SurfaceDataType(d_type)
for d_type in data_types
]
Copy link

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

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

The new list comprehension is indented inconsistently with the surrounding method body and will be reformatted by Black (and may fail formatting checks in CI/pre-commit). Please re-indent this block to Black-style indentation so the comprehension lines align under the opening bracket within the method scope.

Suggested change
d_type if isinstance(d_type, SurfaceDataType)
else SurfaceDataType(d_type)
for d_type in data_types
]
d_type if isinstance(d_type, SurfaceDataType)
else SurfaceDataType(d_type)
for d_type in data_types
]

Copilot uses AI. Check for mistakes.
Comment on lines +692 to +696
data_types = [
d_type if isinstance(d_type, SurfaceDataType)
else SurfaceDataType(d_type)
for d_type in data_types
]
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.

Great. As well as not mutating the container while iterating, it also uses a positive type check.

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.

You don't really need the type check here as suggested in the original issue as Enum(Enum.member) -> Enum.member

@prmukherj
Copy link
Copy Markdown
Collaborator

@Adir-html, could you please fix the code styling issue in this PR. There is a co-pilot suggestion as well, please feel free to commit it.
Thank you.

@Adir-html
Copy link
Copy Markdown
Author

@copilot apply changes based on the comments in this thread

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.

6 participants