Skip to content

Mass autoformat with class and module declarations format fix#12230

Merged
aschackmull merged 11 commits into
github:mainfrom
aschackmull:all/autoformat
Mar 10, 2023
Merged

Mass autoformat with class and module declarations format fix#12230
aschackmull merged 11 commits into
github:mainfrom
aschackmull:all/autoformat

Conversation

@aschackmull

Copy link
Copy Markdown
Contributor

I've split the changes into one commit per language.

@aibaars

aibaars commented Feb 17, 2023

Copy link
Copy Markdown
Contributor

This fails the formatting tests.

@aschackmull

Copy link
Copy Markdown
Contributor Author

This fails the formatting tests.

Well, yes, of course. Otherwise the corresponding autoformatter change wouldn't really be a change, would it?

@aibaars

aibaars commented Feb 17, 2023

Copy link
Copy Markdown
Contributor

This fails the formatting tests.

Well, yes, of course. Otherwise the corresponding autoformatter change wouldn't really be a change, would it?

Ah yes, ignore me;-) I hadn't seen that the actual changes to the formatter haven't been merged yet.

class PlatformVersionAvailabilitySpec extends Synth::TPlatformVersionAvailabilitySpec,
AvailabilitySpec {
AvailabilitySpec
{

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A number of these changes are in generated files. Changes need to be made at the source (schema.py or qlgen.py???) then the files regenerated with bazel run //swift/codegen I think.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure that's necessary? It looks like the relevant codegen has some hook to format its output, and I guess that makes the most sense anyway - it would be brittle to rely on a code generator generating correctly formatted code, when the generator could simply run the autoformatter.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah, the codegen calls out to the autoformatter, IIRC (cc @redsun82)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Awesome, thanks.

@aschackmull aschackmull force-pushed the all/autoformat branch 2 times, most recently from d10dd87 to 7057bca Compare March 3, 2023 11:54
@aschackmull aschackmull added the no-change-note-required This PR does not need a change note label Mar 3, 2023
@aschackmull aschackmull marked this pull request as ready for review March 3, 2023 11:55
@aschackmull aschackmull requested a review from a team March 3, 2023 11:55
@aschackmull aschackmull requested review from a team as code owners March 3, 2023 11:55
@aschackmull aschackmull requested review from erik-krogh and removed request for a team March 3, 2023 11:55
@aschackmull

Copy link
Copy Markdown
Contributor Author

Should be merged once the next CLI with the auto-formatter update lands and CI goes green.

@MathiasVP MathiasVP left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

C/C++ 👍

class DifferentiableFunctionExtractOriginalExpr extends Generated::DifferentiableFunctionExtractOriginalExpr {
}
class DifferentiableFunctionExtractOriginalExpr extends Generated::DifferentiableFunctionExtractOriginalExpr
{ }

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't particularly like the newline when the class body is empty, but this might be a special case that's not worth bikeshedding too much about.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

+1

erik-krogh
erik-krogh previously approved these changes Mar 3, 2023
MathiasVP
MathiasVP previously approved these changes Mar 3, 2023
@aschackmull

Copy link
Copy Markdown
Contributor Author

I tried looking a bit at the "empty body gets printed on its own line" issue, but it turns out to be very tricky to fix in a nice way, because we have to account for the possibility of comments appearing within an empty body. So I'll leave it like this.

geoffw0
geoffw0 previously approved these changes Mar 10, 2023

@geoffw0 geoffw0 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Swift still 👍

@michaelnebel michaelnebel left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

C# 👍

@michaelnebel

Copy link
Copy Markdown
Contributor

@aschackmull : Looks like sync files are broken.

@aschackmull

Copy link
Copy Markdown
Contributor Author

Looks like sync files are broken.

Yeah, for some reason find . \( -name "*.qll" -or -name "*.ql" \) -print0 | xargs -0 codeql query format -i failed to find all the files.

@owen-mc owen-mc left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Still good for Go

@aschackmull aschackmull merged commit 8356991 into github:main Mar 10, 2023
@aschackmull aschackmull deleted the all/autoformat branch March 10, 2023 11:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants