Navigation Menu

Skip to content
This repository has been archived by the owner on Jan 31, 2024. It is now read-only.

Alpha custom types #11

Merged
merged 7 commits into from Sep 23, 2021
Merged

Alpha custom types #11

merged 7 commits into from Sep 23, 2021

Conversation

1devNdogs
Copy link
Contributor

This PR add custom types for requested features: Peers and Block Author information, so they can be used by John in the farmer interface.
s.

  • I have manually imported custom types from a JSON file, generated by subspace.js project.
  • Created types interfaces to inject FarmerId and PocPreDigest to the Polkadot API promise.
  • Added/Replaced Block author information in the console.log. So they can be used now by John needs.

Copy link
Member

@nazar-pc nazar-pc left a comment

Choose a reason for hiding this comment

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

Last commit shouldn't have been here. It is better not to mix formatting PR with feature PR like this one, it makes PR bigger than necessary and harder to review for no reason.

Also please hold on merging it, we need to merge alpha to main first to avoid re-reviewing these feature changes and formatting updates alongside with changes in alpha many more times.

@@ -135,42 +136,43 @@ export const Client = async () => {
client.do.blockSubscription.stop()
},
async start() {
if (!client.api) throw (Error("Api Missing, can't start block subscription yet."))
if (!client.api) throw (Error(`Api Missing, can't start block subscription yet.`))
Copy link
Member

Choose a reason for hiding this comment

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

Why change to template string literal if there is no interpolation?

Copy link
Contributor Author

@1devNdogs 1devNdogs Sep 22, 2021

Choose a reason for hiding this comment

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

Last commit shouldn't have been here. It is better not to mix formatting PR with feature PR like this one, it makes PR bigger than necessary and harder to review for no reason.

Will have this in consideration for future commits and work on formats and cleaning in separate PR's.

Why change to template string literal if there is no interpolation?

Should i just use back slash on the can't single quote?

Copy link
Member

Choose a reason for hiding this comment

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

Hm, is it required by linter? If not, I'd leave the way it was, but it is a minor nitpick.

@jdheeter jdheeter mentioned this pull request Sep 22, 2021
Merged
@jdheeter jdheeter merged commit 2f71cfc into alpha Sep 23, 2021
@jdheeter jdheeter deleted the alpha-custom-types branch September 23, 2021 01:05
Copy link
Contributor

@jdheeter jdheeter left a comment

Choose a reason for hiding this comment

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

looks fine: merged

@nazar-pc
Copy link
Member

I explicitly asked not to merge this before alpha is merged into main either. You just made alpha over 100 lines bigger again 😞

The whole idea was to have reasonably sized PRs that can be merged one by one instead of having a huge one that is hard to review, hard to resolve requested changes and hard to merge. By merging more stuff into alpha we make it even bigger than it was.

Every time a piece of code is changed in existing PR, reviewer needs to:

  1. Analyze changes in isolation
  2. Analyze the whole PR with those changes on top

^ is counter-productive for everyone involved.

@jdheeter
Copy link
Contributor

Sorry for the confusion, In order to resolve your comments on the alpha branch I need the changes from this branch to be merged. Otherwise there will be massive conflicts merging this branch later.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants