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

Fix case warning on derived properties #1929

Merged
merged 1 commit into from
Jun 26, 2021
Merged

Conversation

nitnelave
Copy link
Contributor

Description

Replace the _build suffix with a PropsBuild suffix to respect Camel-case types.

When deriving Properties, the compiler sometimes complains of the
non-Camel-case Foo_build. This fixes the issue.

Checklist

  • I have run cargo make pr-flow
  • I have reviewed my own code
  • I have added tests

@nitnelave
Copy link
Contributor Author

I'm open to a better suggestion instead of PropsBuilder, but I don't know this part of the code well enough to find a better name :)

@siku2
Copy link
Member

siku2 commented Jun 22, 2021

I would prefer if we added attributes to suppress these error messages instead.
In this case #[allow(non_camel_case_types)] would do the trick, but we could cover our bases and do:
#[allow(warnings, clippy::all)] which should silence Clippy and all rustc warnings including non_camel_case_types

@ranile
Copy link
Member

ranile commented Jun 22, 2021

Why not use the right conventions? I'd prefer to keep the code, even generated code, according to the conventions

@nitnelave
Copy link
Contributor Author

+1 to keeping the right conventions.

When deriving `Properties`, the compiler sometimes complains of the
non-Camel-case `Foo_build`. This fixes the issue.
@siku2
Copy link
Member

siku2 commented Jun 22, 2021

Why not use the right conventions? I'd prefer to keep the code, even generated code, according to the conventions

I guess I should've clarified. I totally agree in this case, I don't see a good reason why the ident should be _build here, but this is by far not the only case where the Properties derive macro generates idents that go against conventions.
It would be nice if we could fix all of them in this PR.

For instance, here's one case that deserves the attributes:

/// This step name is descriptive to help a developer realize they missed a required prop
pub fn to_step_name(&self, props_name: &Ident) -> Ident {
Ident::new(
&format!("{}_missing_required_prop_{}", props_name, self.name),
Span::call_site(),
)
}

This type deliberately goes against conventions to draw attention to itself, because it's the only thing a user sees when they don't pass a required prop.
MyProps_missing_required_props_id is more obvious than MyPropsMissingRequiredPropsId

@nitnelave
Copy link
Contributor Author

Fair enough, but in that case it's an error anyways, it's okay if it fails linting :D

@teymour-aldridge
Copy link
Contributor

Maybe "MyProps_missing_required_props_id" should be in all caps to make it even more obvious :D

@siku2
Copy link
Member

siku2 commented Jun 26, 2021

Fair enough, but in that case it's an error anyways, it's okay if it fails linting :D

Well no, the type is named that way regardless of whether the app compile or not. If you managed to get a warning for the _build struct it should also be possible to get one for these structs.

Maybe "MyProps_missing_required_props_id" should be in all caps to make it even more obvious :D

Hmm yeah, that might help.


Anyway, I'm just gonna go ahead and merge this. Further changes can be done in subsequent PRs

@siku2 siku2 merged commit 2e8f52c into yewstack:master Jun 26, 2021
@voidpumpkin voidpumpkin added the A-yew-macro Area: The yew-macro crate label Nov 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-yew-macro Area: The yew-macro crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants