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

Implement syntax enforcment for Pallet<T>(_); #8095

Closed
wants to merge 1 commit into from

Conversation

gui1117
Copy link
Contributor

@gui1117 gui1117 commented Feb 10, 2021

As a follow up on #8091

This PR enforce the new syntax.
For now nothing is enforced on the fields of pallet.

If this PR doesn't get through before 3.0 I think we can close it.

@github-actions github-actions bot added the A0-please_review Pull request needs code review. label Feb 10, 2021
@gui1117 gui1117 added B3-apinoteworthy C1-low PR touches the given topic and has a low impact on builders. labels Feb 10, 2021
@bkchr
Copy link
Member

bkchr commented Feb 10, 2021

Why enforce it?

@gui1117
Copy link
Contributor Author

gui1117 commented Feb 10, 2021

I thought some ppl could make typos like:

#[pallet::pallet]
pub struct Pallet(_i);

And would have preferred a more precise error message than just _i is unknown type or something.

But actually I agree it isn't really needed. I let the PR open if some ppl have strong opinion to have it, otherwise I'll close it.

@bkchr
Copy link
Member

bkchr commented Feb 10, 2021

Ahh. Could you not just allow only _ and if someone tries to use _something you reject it? But, if the people want to use the explicit PhantomData or whatever, let them do that. The compiler will tell them when it is incorrect :D

@shawntabrizi
Copy link
Member

I agree. I do not think enforcing this makes sense.

@gui1117
Copy link
Contributor Author

gui1117 commented Feb 10, 2021

ok, with the 3.0 released, I think we can wait to see if ppl get confused by syntax error and maybe improve on this like you suggested or maybe it is not needed at all.
I close it then.

@gui1117 gui1117 closed this Feb 10, 2021
@shawntabrizi shawntabrizi deleted the gui-enforce-pallet-syntax branch February 11, 2021 21:08
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. C1-low PR touches the given topic and has a low impact on builders.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants