Pallet macro: support pallet::extra_constants renaming

[shaunxw]

The syntax for extra constants is as following, where the constants as function name should be snake cases according to Rust naming convention:

#[pallet::extra_constants]
impl<T: Config> Pallet<T> where $optional_where_clause {
	/// $some_doc
	$vis fn $fn_name() -> $some_return_type {
		...
	}
	...
}

But pallet constants are typically camel cased, as defined in old decl_module and new pallet::constants. pallet::extra_constants should provide a way to rename the constants.

[stale]

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

[emostov]

This is still useful

[ericjmiller]

I'll work on this. Might have some questions though.

[ericjmiller]

Questions:

1. You want constants to remain camel cased and extra_constants to change to snake_case only?

2. Why does casing matter between the extra_constants and constants? Is it so the idents match in

 pub fn expand_constants(def: &mut Def) -> proc_macro2::TokenStream

within frame/support/procedural/src/pallet/expand/constants.rs?

3. What's the relationship between constants and extra_constants? I couldn't find a lot of info on extra_constants.

4. When you say that "extra_constants should provide a way to rename the constants" - do you mean that the expand_constants function should find similar idents with other casings and rename them during expansion to be the same as the expand_constant?

Thanks for any help you can provide.

[shawntabrizi]

cc @thiolliere @KiChjang

[gui1117]

1. You want constants to remain camel cased and extra_constants to change to snake_case only?

Until recently there were only camel case constant, any cases were accepted but the convention was to use camel case.

Maybe we should provide a way to rename the constant independently from the function name like:

#[pallet::extra_constant_name(Foo)]
fn foo() -> u32 {..}

Or maybe we can automatically convert to camel case in the metadata.

2. Why does casing matter between the extra_constants and constants? Is it so the idents match in

Both declares some constant to be part of the pallet metadata https://substrate.dev/rustdocs/latest/frame_metadata/struct.ModuleMetadata.html#structfield
It is 2 way to declare the same stuff.
But not all constant can be declared using the 1st way pallet::constant this is why pallet::extra_constant exist:

• pallet::constant allow to put in the constants metadata any associated type that implement Get<$type> for the trait Config.
• pallet::extra_constant allow to put in the constants metadata any value, by declaring a function returning the given value.

3. What's the relationship between constants and extra_constants? I couldn't find a lot of info on extra_constants.

extra_constant information is https://substrate.dev/rustdocs/latest/frame_support/attr.pallet.html#extra-constants-palletextra_constants-optional
It is only an additional way to declare some constant.

4. When you say that "extra_constants should provide a way to rename the constants" - do you mean that the expand_constants function should find similar idents with other casings and rename them during expansion to be the same as the expand_constant?

I think by providing a way to rename the constant he meant have some additional syntax which allows to specify the name of the constant. like:

#[pallet::extra_constant]
impl<T: Config> Pallet<T> {
  #[pallet::constant_name(Foo)]
  fn foo() -> u32 {..}
}

[ericjmiller]

After thinking about this, I like the idea of an inert attribute, as @thiolliere's last example demonstrates. This way, extra_constant names are explicit and more configurable. Also, I think it will be more flexible for future changes.

Plus, I don't like convention when you can be explicit. So if a user wants to break away from convention for some reason, that should probably be allowed. And it should be optional, so that if no inert attr is passed, the function name is taken.

Let me know if you agree/disagree. I'll put out a PR in the meantime...

[gui1117]

yes I agree, I also think we can make this attribute optional so that we don't break existing pallets

[ericjmiller]

Added failing tests in draft above. I'll be adding logic to extra_constants.rs and helper.rs, such that when pallet:: Attributes are discovered, a new ExtraConstantDef is created with ident being the Attribute token rather than the method ident.

This will, of course, be optional.

[ericjmiller]

I've updated the draft PR and could use some feedback.

I've passed the failed tests, but had to comment the code that explicitly initializes ExtraConstantDef with the new ident. It's very odd.

Most of the commented code should work in a final solution to this, but I think perhaps something weird is going on in the tests.