Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Comments

Refactor sr-api to not depend on client anymore#4086

Merged
gnunicorn merged 4 commits intomasterfrom
bkchr-sr-api-refactoring
Nov 11, 2019
Merged

Refactor sr-api to not depend on client anymore#4086
gnunicorn merged 4 commits intomasterfrom
bkchr-sr-api-refactoring

Conversation

@bkchr
Copy link
Member

@bkchr bkchr commented Nov 11, 2019

No description provided.

@bkchr bkchr added A0-please_review Pull request needs code review. A2-insubstantial Pull request requires no code review (e.g., a sub-repository hash update). labels Nov 11, 2019
@bkchr bkchr removed the A2-insubstantial Pull request requires no code review (e.g., a sub-repository hash update). label Nov 11, 2019
@tomaka
Copy link
Contributor

tomaka commented Nov 11, 2019

I know we want to go fast, but I'm not a fan of marking a PR that touches 2500 lines of code as "insubstantial" and without any description.

@bkchr
Copy link
Member Author

bkchr commented Nov 11, 2019

That was by mistake and I already have removed that (before your comment :P).

Copy link
Contributor

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

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

lgtm, few minor grumbles

@@ -1,4 +1,4 @@
// Copyright 2017-2019 Parity Technologies (UK) Ltd.
// Copyright 2018-2019 Parity Technologies (UK) Ltd.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the previous line changed to 2019 only while here we bumped the year?

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably some copy paste error by me.

use header_metadata::{HeaderMetadata, CachedHeaderMetadata};

use sr_api::{CallRuntimeAt, ConstructRuntimeApi, Core as CoreApi, ProofRecorder, InitializeBlock};

Copy link
Contributor

Choose a reason for hiding this comment

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

Please let's not do this empty lines. The grouping logic is arbitrary and not clear, I'd rather have a single block sorted alphabetically.

/// Error while working with inherent data.
#[display(fmt="InherentData error: {}", _0)]
InherentData(String),
#[display(fmt="InherentData error: {:?}", _0)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be a messy output, why doesn't inherents::Error implement Display?

Copy link
Member Author

Choose a reason for hiding this comment

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

It implements it now, but not at the point where I made this change.

#[display(fmt = "Creating inherents failed: {}", _0)]
CreateInherents(RuntimeString),
#[display(fmt = "Creating inherents failed: {:?}", _0)]
CreateInherents(inherents::Error),
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

Co-Authored-By: Tomasz Drwięga <tomusdrw@users.noreply.github.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

A0-please_review Pull request needs code review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants