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

feat: use new IterableMap and IterableSet collections #139

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

aleksuss
Copy link
Collaborator

@aleksuss aleksuss commented Jul 15, 2024

TODO: Check backward compatibility.

@Near-One Near-One deleted a comment Aug 17, 2024
@Near-One Near-One deleted a comment Aug 17, 2024
@Near-One Near-One deleted a comment Aug 17, 2024
@karim-en
Copy link
Collaborator

@aleksuss Looks like the changes are backward compatible. We have added a test here, please merge with master to check it out.

@aleksuss aleksuss closed this Aug 26, 2024
@aleksuss aleksuss reopened this Aug 26, 2024
@aleksuss
Copy link
Collaborator Author

Looks like the changes are backward compatible.

@karim-en But the test says the opposite.

@karim-en
Copy link
Collaborator

karim-en commented Aug 26, 2024

Looks like the changes are backward compatible.

@karim-en But the test says the opposite.

Sorry it was a typo. I wanted to say "aren't backward compatible"

@aleksuss
Copy link
Collaborator Author

I thought to add these changes to the 0.3.0 version, but as I can see, this version has already been released.

@karim-en
Copy link
Collaborator

I thought to add these changes to the 0.3.0 version, but as I can see, this version has already been released.

It is dangerous to do a release like this because the plugin's users would like to upgrade to the latest version and miss the plugins being upgradable and needing a migration.
What I can propose is adding a non-default feature flag for these changes.

@aleksuss
Copy link
Collaborator Author

Changing a minor version if the major is 0 implies the existence of breaking changes by semver rules. As for me, I don't like a feature flag. I would postpone these changes to the next release though.

@karim-en
Copy link
Collaborator

karim-en commented Aug 26, 2024

Changing a minor version if the major is 0 implies the existence of breaking changes by semver rules. As for me, I don't like a feature flag. I would postpone these changes to the next release though.

It is storage breaking changes, we can't allow releasing things like this without proper storage migration implementation.
The breaking changes are acceptable just on the code or API level.

Check these contracts
https://github.com/near/near-sdk-rs/tree/master/near-contract-standards

@karim-en
Copy link
Collaborator

karim-en commented Aug 26, 2024

Changing a minor version if the major is 0 implies the existence of breaking changes by semver rules. As for me, I don't like a feature flag. I would postpone these changes to the next release though.

It is storage breaking changes, we can't allow releasing things like this without proper storage migration implementation. The breaking changes are acceptable just on the code or API level.

Also, IMHO I'm skeptical about the performance improvements of the store crate; I've checked the code, and there is a scenario where theses collections introduce an extra clone

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.

3 participants