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

Rename Compact to Portable #41

Merged
merged 16 commits into from Jan 4, 2021
Merged

Rename Compact to Portable #41

merged 16 commits into from Jan 4, 2021

Conversation

dvdplm
Copy link
Contributor

@dvdplm dvdplm commented Dec 17, 2020

Based on #35 (aj-compact-string).

This PR changes the terminology from compact into frozen, which is useful to avoid confusion with the terminology used in parity-scale-codec, where the Compact type has a prominent role (c.f. #8).

"Frozen" is a sub-optimal name. Other candidates include: Universal (too vague), Static (confusing), Standalone (too vauge), Decontexualized (is that even a word?), Fixed (seems to imply something about space usage), Exported (that's not the key aspect though is it?), Serializable (kind of confusing with serde being in the mix).

Also included: a bunch of edits to the documentation in the hope of improving readability a bit.

ascjones and others added 11 commits December 7, 2020 11:49
By default, parity-scale-codec does not provide Encode/Decode impls for an owned String.
This is only provided under the "full" feature which is not used by the substrate runtime,
because it should not be used for consensus critical code. So in order for the CompactForm
to be integrated into the substrate runtime, or wherever the "full" feature cannot be used,
then we must parameterize the `String` type so that it can be both an `&'static str` on the
runtime side where it is encoded, and a `String` in client/consuming code where it is decoded.
Add a `compact` member to `Field` to indicate it is `scale_codec::Compact`
@dvdplm dvdplm marked this pull request as ready for review December 17, 2020 18:53
@dvdplm dvdplm requested a review from ascjones December 17, 2020 18:53
@dvdplm dvdplm self-assigned this Dec 17, 2020
@ascjones
Copy link
Member

ascjones commented Jan 4, 2021

What about "Portable"?

@dvdplm
Copy link
Contributor Author

dvdplm commented Jan 4, 2021

Portable works for me. Not ideal either (and it's not a movie) but it's better than Frozen. @Robbepop any comment on this before I unleash grep?

@Robbepop
Copy link
Collaborator

Robbepop commented Jan 4, 2021

Portable works for me. Not ideal either (and it's not a movie) but it's better than Frozen. @Robbepop any comment on this before I unleash grep?

Cannot come up with a better idea for another name. Names never bothered me anyway.

@dvdplm dvdplm changed the title Rename Compact to Frozen Rename Compact to Portable Jan 4, 2021
@dvdplm
Copy link
Contributor Author

dvdplm commented Jan 4, 2021

@ascjones ptal

Copy link
Member

@ascjones ascjones left a comment

Choose a reason for hiding this comment

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

LGTM

src/lib.rs Outdated
//! There is an uncompact form, called [`MetaForm`](`crate::form::MetaForm`) that acts as a bridge
//! from compile-time type information at runtime in order to easily retrieve all information needed
//! to uniquely identify types.
//! There is an expanded form, called [`MetaForm`](`crate::form::MetaForm`) that
Copy link
Member

Choose a reason for hiding this comment

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

I don't think expanded is the right word here. That said, I'm not sure what the right word would be here either. Could maybe just remove There is an expanded form and just use the rest of the sentence. Or something to describe that this is how types are described and identified statically.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The whole paragraph is a bit weird imo. Hard to rewrite because I'm not sure I understand it myself. Changing to:

//! # Forms
//!
//! To bridge between compile-time type information and runtime the
//! [`MetaForm`](`crate::form::MetaForm`) is used to easily retrieve all
//! information needed to uniquely identify types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Feel free to suggest moar betterments, happy to make the changes.

@dvdplm dvdplm merged commit 138e351 into master Jan 4, 2021
@dvdplm dvdplm deleted the dp-rename-compact-to-frozen branch January 4, 2021 19:04
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.

None yet

3 participants