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

Update to base/Buffer version 0.8.7 #10

Closed
wants to merge 3 commits into from

Conversation

f0i
Copy link
Contributor

@f0i f0i commented Apr 20, 2023

I updated the StableBuffer based on the current version 0.8.7 published on MOPS.
There is currently no tag for 0.8.7 in dfinity/motoko-base so it could also be 0.8.6 🤔. Anyway, the version in HEAD matches the one published with MOPS, and the one now in reference/Buffer.mo.

Test cases have been slightly changed to reflect the new capacity expansion factor of 1.5x.

Code has been formatted by the official Motoko VS-Code plugin/formatter which unfortunately causes a noisy diff.
I could attempt to create a distinct commit for the formatting if it poses an issue. However, if it's not a concern for you, I won't invest time in it.

@f0i
Copy link
Contributor Author

f0i commented Apr 20, 2023

There is currently no tag for 0.8.7 in dfinity/motoko-base so it could also be 0.8.6 .

According to mops_one, the base library was taken from https://github.com/dfinity/motoko/releases.
So it is actually 0.8.7.

Copy link
Contributor

@ByronBecker ByronBecker left a comment

Choose a reason for hiding this comment

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

@f0i Thanks for this PR and work!

This is a huge PR, so it's going to take me a bit of time to go through everything.

Two suggestions that would make this a whole lot easier on my end to review would be:

  1. Split this into 4-5 different small PRs. Put 15 functions in each one, and I'll review promptly. We should finish in 1-2 weeks.
  2. Match up the diffs, so that previously existing functions are in the same order at the top, and everything new comes after it below (I can review functions side by side).

Comment on lines +50 to +51
var _size : Nat; // avoid name clash with `size()` method
var elements : [var ?X];
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a complete breaking change with the StableBuffer type. Since this is now stable, the inner properties are expected to be the same name before & after upgrade.

Can you revert this to keep the original property names?

This applies to everywhere else that initCapacity, count, and elems are referenced in the PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess the alternative is that we release a StableBufferV2 repo, which is recommended as it is more in sync with the one in base. That actually might be easier, and then we don't break people.

@f0i
Copy link
Contributor Author

f0i commented Apr 27, 2023

I'll close this one and create separate pull requests as recommended above.

The first one is #11 which updates the functions that already existed in the previous version.

Also, the StableBuffer type will be identical to the previous version of StableBuffer to maintain backward compatibility.

@f0i f0i closed this Apr 27, 2023
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