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

Add field names to pallet Event variants #1277

Merged
merged 15 commits into from
Feb 21, 2022

Conversation

warfollowsme
Copy link
Contributor

Description

This PR changes/added:

  • pallet events variants from tuples to structs with named fields;
  • descriptions for some events.

After PR#9896 was merged events can now be described with field names.

These field names and their respective comments are in turn reflected in the metadata for a better client experience.

@crystalin
Copy link
Collaborator

@warfollowsme Thank you for your contribution, it looks good.
Can you cargo fmt the code please ?

@warfollowsme
Copy link
Contributor Author

@crystalin Do I need fixed all diffs from the build log too?

@librelois librelois added the B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes label Feb 10, 2022
@librelois
Copy link
Collaborator

librelois commented Feb 10, 2022

@crystalin Do I need fixed all diffs from the build log too?

No need to fix them manually, you just need to run cargo fmt :)

@warfollowsme
Copy link
Contributor Author

warfollowsme commented Feb 10, 2022

No need to fix them manually, you just need to run cargo fmt :)

okay. done. :)

@warfollowsme
Copy link
Contributor Author

Okay. I realized that I completely forgot to change the use of events in tests.

@warfollowsme
Copy link
Contributor Author

build workflow is done. what should I do to fix the typescript tests?

@librelois
Copy link
Collaborator

build workflow is done. what should I do to fix the typescript tests?

You probably just need to merge master

@warfollowsme
Copy link
Contributor Author

warfollowsme commented Feb 20, 2022

You probably just need to merge master

Okay. Only docker compile left. Issues with login. How can I fix it?
Also, these workflows are not required, perhaps you can merge PR.

@librelois
Copy link
Collaborator

Okay. Only docker compile left. Issues with login. How can I fix it? Also, these workflows are not required, perhaps you can merge PR.

You can't fix it, we need to skip theses jobs for external PRs. @crystalin can you merge this PR ?

@crystalin
Copy link
Collaborator

@librelois if this is inconsistent with our other pallets, I'd prefer to have them all included the same format before merging it (to avoid having multiple breaking changes release) what do you think ?

@crystalin
Copy link
Collaborator

Actually, does it impact the clients (polkadotJS UI or SDK) ? It doesn't seem so looking at the file changed. if not I'll merge it

@librelois
Copy link
Collaborator

@librelois if this is inconsistent with our other pallets, I'd prefer to have them all included the same format before merging it (to avoid having multiple breaking changes release) what do you think ?

We have a lot of events with unnamed fields (basically everywhere…), I think it would be a too big PR to change everything at once, it's better to do it in several times :)

@librelois
Copy link
Collaborator

Actually, does it impact the clients (polkadotJS UI or SDK) ? It doesn't seem so looking at the file changed. if not I'll merge it

I know that polkadotjs is not impacted, for the rest I don't know

@crystalin
Copy link
Collaborator

Merging, thank you @warfollowsme for your contribution

@crystalin crystalin merged commit 409597b into moonbeam-foundation:master Feb 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants