Skip to content

Conversation

@xclud
Copy link
Contributor

@xclud xclud commented Mar 16, 2023

This is equivalent to running dart format . in the directory after the code is generated.

Usually I get warnings from the generated code (Which can be turned of by the linter, i know). Also, If the code is uploaded as a package to pub.dev, It will lose 10 points for formatting.

Pass static analysis

code has no errors, warnings, lints, or formatting issues.

dev_dependencies:
collection: ^1.15.0
lints: ^1.0.0
lints: ^2.0.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

revert these changes!

if (!_linked) throw StateError('not linked');

String formatDartCode(String code) {
final formatter = DartFormatter();
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe make the formater as instance var? So it's not created every call?

Copy link
Member

@osa1 osa1 left a comment

Choose a reason for hiding this comment

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

We had discussed this PR before with @sigurdm. The problem here is that this formats based on the formatter used by the plugin, instead of the project's formatter, so dart format may format differently than protoc_plugin, and since you will be using dart format to format rest of your project anyway, you will be reformatting the generated files.

So I think this isn't solving any problems and no need to add a dependency to the project and make build times slower.

We also had a relevant PR in the past: #702. When the gprc support was added to this library (in the commit linked in the PR) formatting was added to the open source version, but not to the internal version. It wasn't documented in the commit (or the CL) why this was done differently but perhaps there are other reasons why we don't want to format, at least internally.

@osa1
Copy link
Member

osa1 commented Oct 10, 2023

Closing, for the reasons described above.

@osa1 osa1 closed this Oct 10, 2023
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.

3 participants