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

Make tests build #429

Merged
merged 7 commits into from
May 3, 2023
Merged

Make tests build #429

merged 7 commits into from
May 3, 2023

Conversation

ggwpez
Copy link
Member

@ggwpez ggwpez commented May 3, 2023

Tests do not build with only MEL feature as pointed out in #428 (review)

This standard pattern that is used multiple times in the code does not actually work:

#[cfg(not(feature = "derive"))]
use parity_scale_codec_derive::Decode;
use parity_scale_codec::Decode;

It errors with a redefinition of Decode when the feature is disabled. I aliased them in the tests now to DeriveDecode.
It is a bit unfortunate that parity_scale_codec::Decode is both a trait and a derive macro, depending on the features.

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@ggwpez ggwpez requested a review from a team as a code owner May 3, 2023 09:18
ggwpez added 2 commits May 3, 2023 12:16
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@ggwpez ggwpez changed the title WIP: Fix build WIP: Make tests build May 3, 2023
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@ggwpez ggwpez changed the title WIP: Make tests build Make tests build May 3, 2023
@ggwpez ggwpez requested review from bkchr and mrcnski May 3, 2023 12:44
src/lib.rs Outdated
//!
//! use parity_scale_codec::{Encode, Decode, Compact, HasCompact};
//!
//! #[derive(Debug, PartialEq, Encode, Decode)]
//! #[derive(Debug, PartialEq, DeriveEncode, DeriveDecode)]
Copy link
Member

Choose a reason for hiding this comment

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

I'm not happy with this. People will be confused on where DeriveEncode is coming from.

https://users.rust-lang.org/t/how-to-document-optional-features-in-api-docs/64577/3 above the code example with feature = "derive" would be really nice!

Copy link
Member Author

@ggwpez ggwpez May 3, 2023

Choose a reason for hiding this comment

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

This auto-generated feature warning seems to only work on whole modules or types. Not on single doc lines, so i added:

#![cfg_attr(feature = "derive", doc = "```rust")]
#![cfg_attr(not(feature = "derive"), doc = "*(Only compiled with feature `derive`)*\n```ignore")]

To each example to not compile them without the feature. Works also on stable.

tests/mod.rs Show resolved Hide resolved
ggwpez added 2 commits May 3, 2023 17:31
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Copy link
Contributor

@mrcnski mrcnski left a comment

Choose a reason for hiding this comment

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

It errors with a redefinition of Decode when the feature is disabled.

Instead of the macro import could you have renamed the trait import, or even brought it in without a name, e.g. use Decode as _? But this is fine too.

.gitlab-ci.yml Outdated Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
Co-authored-by: Marcin S. <marcin@realemail.net>
@ggwpez
Copy link
Member Author

ggwpez commented May 3, 2023

Instead of the macro import could you have renamed the trait import, or even brought it in without a name, e.g. use Decode as _?

I tried both, but the trait and the derive macro have the same name. So when i import the trait as _, then the derive macro also becomes unavailable 🤦‍♂️
And when i import the derive macro as _ from the derive crate, then it is not usable.

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