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

class Struct extends Map #321

Merged
merged 18 commits into from Oct 19, 2018
Merged

class Struct extends Map #321

merged 18 commits into from Oct 19, 2018

Conversation

amaury1093
Copy link
Contributor

@amaury1093 amaury1093 commented Oct 18, 2018

Related #161.

Instead of doing all types at once, let's do one by one, so that it's an easier transition. During this transition, I still added this.raw = this; in the Struct class.

This is one of the bigger ones. Breaking changes:

  • Struct.keys() returns an iterator, not an array
  • Struct.values() returns an iterator, not an array

TODO:

  • I'll just update @polkadot/dev for the jest testMatch, so that I remove that from this PR

@amaury1093 amaury1093 added WIP Work in Progress @types labels Oct 18, 2018
@@ -147,19 +147,20 @@ export default class Method extends Struct {
}

get args (): Array<Base> {
return (this.getAtIndex(1) as Struct<Base>).values();
// FIXME This should return a Struct instead of an Array
return [...(this.get('args') as Struct).values()];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jacogr I think in the UI you use method.args() as an array. So Map or Array?

Copy link
Member

Choose a reason for hiding this comment

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

Prefer array. Plus it is actually closer to .values()

@jacogr
Copy link
Member

jacogr commented Oct 18, 2018

@amaurymartiny dev PR merged.

Copy link
Member

@jacogr jacogr left a comment

Choose a reason for hiding this comment

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

Looks good

@@ -16,15 +16,15 @@ export class RxProposal extends Struct.with({ id: PropIndex, proposal: Proposal,
}

get address (): AccountId {
return this.raw.address as AccountId;
return this.get('address') as AccountId;
Copy link
Member

Choose a reason for hiding this comment

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

+1

I def prefer this - less magic.

// @ts-ignore FIXME
value[jsonKey]
);
raw[key] = value[jsonKey as string] instanceof Types[key]
Copy link
Member

Choose a reason for hiding this comment

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

Nice! I really tried to fix that :)

@amaury1093 amaury1093 removed the WIP Work in Progress label Oct 18, 2018
@jacogr jacogr merged commit f2287f3 into master Oct 19, 2018
@jacogr jacogr deleted the am-codec branch October 19, 2018 12:49
@polkadot-js-bot
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@polkadot-js polkadot-js locked as resolved and limited conversation to collaborators Jun 9, 2021
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