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

style: simplify some statements for readability #1962

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

hamirmahal
Copy link
Contributor

Closes #1961

@hamirmahal hamirmahal force-pushed the style/simplify-some-statements-for-readability branch from f2221d7 to 064f167 Compare December 15, 2024 23:47
Copy link
Contributor Author

@hamirmahal hamirmahal left a comment

Choose a reason for hiding this comment

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

I updated rustup to match the version in CI. This should fix failing formatting checks.

Comment on lines 125 to 128
type As<'src> = &'src str where Self: 'src;
type As<'src>
= &'src str
where
Self: 'src;
Copy link
Member

Choose a reason for hiding this comment

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

...I would prefer not to?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's how the version of rustup in CI formats things.

Copy link
Member

Choose a reason for hiding this comment

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

Ugh.

Copy link
Member

Choose a reason for hiding this comment

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

Can these have #[rustfmt::skip] instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm... I'll take a look right now.

@hamirmahal hamirmahal force-pushed the style/simplify-some-statements-for-readability branch from 064f167 to 16a789f Compare December 17, 2024 22:50
Comment on lines 618 to 624
let sql_graph_entity_fn_name = syn::Ident::new(
&format!("__pgrx_internals_aggregate_{}", snake_case_target_ident),
&format!("__pgrx_internals_aggregate_{snake_case_target_ident}"),
target_ident.span(),
Copy link
Member

Choose a reason for hiding this comment

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

All the cases where this Ident::new pattern is used should remain the same for reasons that are somewhat complex and have to do with "this pattern is suboptimal, but dtolnay/quote#206 is too, and it's not just a formatting difference."

@workingjubilee
Copy link
Member

Tests can be fixed with TRYBUILD=overwrite

@hamirmahal hamirmahal force-pushed the style/simplify-some-statements-for-readability branch from 16a789f to e7fd0ef Compare December 18, 2024 22:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Some statements can be simplified for readability
2 participants