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 codec to 2.0 and release new version (before primitive-types release a new version for codec 2.0) #55

Merged
merged 4 commits into from
Jan 27, 2021

Conversation

gui1117
Copy link
Contributor

@gui1117 gui1117 commented Jan 26, 2021

I'm not sure about this version bump:

substrate doesn't depend on scale-info, but primitive-types does depend optionally on scale-info, and primitive-types will get soon upgraded to 2.0 thus I think it is better if it also update scale-info to 0.5.

Apart from this, it seems we don't need to bump the derive crate, but I'm not sure, maybe it is better not to mix different version of scale-codec when deriving scale-info.
you can take a look at the changelog to make your mind https://github.com/paritytech/parity-scale-codec/blob/master/CHANGELOG.md

@gui1117 gui1117 requested review from ascjones and Robbepop January 26, 2021 18:28
@gui1117 gui1117 changed the title Update codec to 2.0 and release new version Update codec to 2.0 and release new version (before primitive-types release a new version for codec 2.0) Jan 26, 2021
@gui1117
Copy link
Contributor Author

gui1117 commented Jan 26, 2021

I can't understand why CI is failing though

@dvdplm
Copy link
Contributor

dvdplm commented Jan 27, 2021

I can't understand why CI is failing though

I think it's because rustc is really old (see

cargo +nightly-2020-03-11 build --no-default-features
).

@ascjones
Copy link
Contributor

substrate doesn't depend on scale-info, but primitive-types does depend optionally on scale-info, and primitive-types will get soon upgraded to 2.0 thus I think it is better if it also update scale-info to 0.5.

Yes we should do a release here, though I'd like to get #53 included so I'll review that now.

Apart from this, it seems we don't need to bump the derive crate

I believe we will need to for the release because there are definitely changes in there since the last release e.g. #30

@dvdplm
Copy link
Contributor

dvdplm commented Jan 27, 2021

I think it's because rustc is really old

No, I'm wrong (well, it is really old), I think you forgot to update this.

@ascjones
Copy link
Contributor

I think it's because rustc is really old

#56

@gui1117
Copy link
Contributor Author

gui1117 commented Jan 27, 2021

I think it's because rustc is really old

No, I'm wrong (well, it is really old), I think you forgot to update this.

Yes CI is successful now

Copy link
Contributor

@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. We can bump the derive crate as part of a PR which updates the CHANGELOG

@gui1117
Copy link
Contributor Author

gui1117 commented Jan 27, 2021

LGTM. We can bump the derive crate as part of a PR which updates the CHANGELOG

Do you mean you prefer to bump derive crate ? then I can do here it is fine

@dvdplm
Copy link
Contributor

dvdplm commented Jan 27, 2021

I'd be in favour of bumping it here, but either way is fine.

@ascjones
Copy link
Contributor

LGTM. We can bump the derive crate as part of a PR which updates the CHANGELOG

Do you mean you prefer to bump derive crate ? then I can do here it is fine

Yes please

@gui1117
Copy link
Contributor Author

gui1117 commented Jan 27, 2021

Yes done

@ascjones ascjones merged commit e466493 into master Jan 27, 2021
@ascjones ascjones deleted the gui-update-codec branch January 27, 2021 11:49
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