Skip to content

Migrate examples to new APIs #487

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

Merged
merged 8 commits into from
Sep 21, 2020
Merged

Conversation

cmichi
Copy link
Collaborator

@cmichi cmichi commented Sep 17, 2020

Closes #485

Left these two ToDos out:

Migrate examples to use the ops which were introduced for storage2::Vec in #453

There are no examples which could use Vec::set() or Vec::clear() at the moment.

If possible, migrate examples to use storage2::Stash::remove_occupied(…) introduced in #440

The method is unsafe and we try to use only unsafe in examples when it's strictly necessary.

In terms of the new HashMap Entry API there are more places where we could use them ‒ at the cost of readability though.

@cmichi cmichi requested a review from Robbepop September 17, 2020 12:17
@cmichi
Copy link
Collaborator Author

cmichi commented Sep 17, 2020

This is an example of a code snippet which would get more complicated using the new Entry API: https://github.com/paritytech/ink/blob/d4df4e7ebadc449fc6d63cebb115d546d3b57ca5/examples/erc721/lib.rs#L266-L274.

@codecov-commenter
Copy link

codecov-commenter commented Sep 17, 2020

Codecov Report

Merging #487 into master will decrease coverage by 14.07%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #487       +/-   ##
===========================================
- Coverage   80.11%   66.03%   -14.08%     
===========================================
  Files         142      142               
  Lines        6230     6230               
===========================================
- Hits         4991     4114      -877     
- Misses       1239     2116      +877     
Impacted Files Coverage Δ
lang/macro/src/lib.rs 0.00% <0.00%> (-100.00%) ⬇️
lang/macro/src/lint.rs 0.00% <0.00%> (-100.00%) ⬇️
lang/macro/src/contract.rs 0.00% <0.00%> (-100.00%) ⬇️
lang/macro/src/codegen/mod.rs 0.00% <0.00%> (-100.00%) ⬇️
lang/macro/src/codegen/events.rs 0.00% <0.00%> (-100.00%) ⬇️
lang/macro/src/codegen/storage.rs 0.00% <0.00%> (-100.00%) ⬇️
lang/macro/src/codegen/contract.rs 0.00% <0.00%> (-100.00%) ⬇️
lang/macro/src/codegen/cross_calling.rs 0.00% <0.00%> (-98.31%) ⬇️
lang/macro/src/codegen/dispatch.rs 0.00% <0.00%> (-97.86%) ⬇️
lang/macro/src/codegen/metadata.rs 0.00% <0.00%> (-93.98%) ⬇️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c22562a...d2b97fe. Read the comment docs.

Copy link
Collaborator

@Robbepop Robbepop left a comment

Choose a reason for hiding this comment

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

All in all looks good but contains some non semantic preserving changes that should either be rewritten or removed.

Copy link
Collaborator

@Robbepop Robbepop left a comment

Choose a reason for hiding this comment

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

LGTM

@cmichi cmichi merged commit 626bfc2 into master Sep 21, 2020
@cmichi cmichi deleted the cmichi-migrate-examples-to-ink3 branch September 21, 2020 19:34
@TriplEight TriplEight mentioned this pull request Oct 2, 2020
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.

Migrate examples to ink! 3.0
3 participants