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

Extract ink_core/memory into its own sub-crate #143

Closed
wants to merge 9 commits into from

Conversation

ascjones
Copy link
Collaborator

Resolves #110.

For now I've reexported ink_memory as memory from ink_core, so all existing imports are valid. Undecided about whether to directly import ink_memory for usages in core and model.

@ascjones ascjones added the A-ink_storage [ink_storage] Work Item label Jul 15, 2019
@ascjones ascjones requested a review from Robbepop July 15, 2019 12:17
@codecov-io
Copy link

codecov-io commented Jul 15, 2019

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #143      +/-   ##
==========================================
- Coverage   86.33%   86.22%   -0.12%     
==========================================
  Files          60       60              
  Lines        4830     5125     +295     
==========================================
+ Hits         4170     4419     +249     
- Misses        660      706      +46
Impacted Files Coverage Δ
core/src/lib.rs 100% <ø> (ø) ⬆️
lang/src/tests/events.rs 100% <0%> (ø) ⬆️
lang/src/tests/incrementer.rs 100% <0%> (ø) ⬆️
lang/src/tests/noop.rs 100% <0%> (ø) ⬆️
lang/src/tests/flipper.rs 100% <0%> (ø) ⬆️
core/src/env/srml/types.rs 0% <0%> (ø) ⬆️
lang/src/api.rs 91.78% <0%> (+1.04%) ⬆️
core/src/env/test_env.rs 45.38% <0%> (+3.8%) ⬆️
lang/src/gen/build.rs 85.57% <0%> (+3.95%) ⬆️

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 7d19156...64b9e85. Read the comment docs.

@Robbepop
Copy link
Collaborator

According to our new guidelines every crate requires a README that (shortly) describes what it is about.

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.

Missing a README

@ascjones
Copy link
Collaborator Author

So not just a symlink to the root README like in the other crates?

@Robbepop
Copy link
Collaborator

So not just a symlink to the root README like in the other crates?

That's a good question.
I think it would be best if we provide a small description with rationals for the subcrate's existence for all the minor sub crates and then a link to the major README. What do you think?

@ascjones
Copy link
Collaborator Author

As an experiment, I'm generating the README with cargo readme. It will use the crate module level docs and the template to generate the README.

If we like this idea we can use it for other crates and use a shared template, and perhaps some automation to generate the READMEs.

@ascjones
Copy link
Collaborator Author

@Robbepop does that seem a reasonable solution for the README?

@Robbepop
Copy link
Collaborator

I really like the direction of separating this functionality out into its own sub-crate, however, I think we can do even better than simply moving the bits over. This could potentially act as a std to no_std + alloc compatibility layer instead of just an alloc shim.

So all std pieces would always be loaded from within this sub-crate.
For the purpose of usability we should also provide a prelude that we can simply import.
Also maybe the name of this compatibility layer should be different from ink_memory.

What about ink_std? - That's may be a bit confusing due to ink_core existence though ...
Maybe later we can also split apart ink_core into ink_env and ink_storage.
Also we might want to add an ink_traits for bridging between the different sub-crates. At this point I should probably just open a new issue ...

@ascjones
Copy link
Collaborator Author

See #157

@ascjones ascjones closed this Jul 30, 2019
@ascjones ascjones deleted the aj-memory-crate branch November 25, 2019 11:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ink_storage [ink_storage] Work Item
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extract ink_core/memory into its own sub-crate
3 participants