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

allow for mappings to structs in BackupDataRetriever #345

Merged
merged 7 commits into from
Nov 22, 2024

Conversation

lydiagarms
Copy link
Contributor

No description provided.

@lydiagarms lydiagarms marked this pull request as ready for review November 20, 2024 12:29
@lydiagarms lydiagarms requested a review from kKahina November 22, 2024 11:50
Copy link
Contributor

Choose a reason for hiding this comment

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

I see there are two consecutive if statements. Could you explain why this separation is necessary instead of combining them into a single if with multiple conditions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previously the if statement was inside else because we didn't have both structs and mappings at the same time. Now we are supporting mappings to structs and so we want to increment varName by s even if mappingKey is true.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the clarification

Copy link
Contributor

Choose a reason for hiding this comment

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

Does the new code properly handle the state variable indices and structure properties without causing any issues or inconsistencies in the logic ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes as far as I'm aware, can you see any problems?

Copy link
Contributor

Choose a reason for hiding this comment

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

I’ll go ahead and approve it

@lydiagarms lydiagarms merged commit 968b645 into master Nov 22, 2024
2 checks passed
Copy link

🎉 This PR is included in version 1.5.28 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants