Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add StructureBuilder missing python methods #5308

Merged
merged 5 commits into from
May 9, 2024

Conversation

rbran
Copy link
Contributor

@rbran rbran commented Apr 22, 2024

No description provided.

@rbran
Copy link
Contributor Author

rbran commented Apr 22, 2024

Could the non-mutable setters be problematic? What if the user hold a reference to the builder (or internals) before a setter call?

Maybe a code like this could cause problems:

let builder: StructureBuilder = todo!();
let members = builder.members();
builder.clear_members();
for member in members.iter() {
 ...
}

This could be solved by having all the setters functions require &mut self and return an Array tied to the self lifetime. Or simply clone all the members and return a Vec<StructureMember> instead.

@ElykDeer ElykDeer force-pushed the structure-builder-methods branch from 166ba02 to d9b6a55 Compare May 9, 2024 16:40
@ElykDeer
Copy link
Member

ElykDeer commented May 9, 2024

This isn't building locally for me... Complaining about CoreArrayWrapper not accepting a lifetime parameter. Not sure why it works in CI, though. I'll look into this more later.

@rbran
Copy link
Contributor Author

rbran commented May 9, 2024

This was written before the GAT PR, I'll fix this.

@rbran
Copy link
Contributor Author

rbran commented May 9, 2024

@KyleMiles Fixed.

@ElykDeer ElykDeer merged commit 71a1c7e into Vector35:dev May 9, 2024
2 checks passed
@rbran rbran deleted the structure-builder-methods branch May 9, 2024 17:33
@ElykDeer ElykDeer self-assigned this May 10, 2024
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