Skip to content

Conversation

@tarrinneal
Copy link
Contributor

Fixes all lint issues mentioned in flutter/flutter#85321.

I'm not sure that these are all changes that will be agreed upon, but I thought I'd put them out there for discussion.

List which issues are fixed by this PR. You must list at least one issue.
fixes flutter/flutter#85321

This change is breaking due to the newly final java classes being generated. I wouldn't expect this to effect many users though.

@gaaclarke
Copy link
Member

Hey @tarrinneal did we hear back from internal customers that these linter issues are still worth fixing? @ChristianEdwardPadilla might have some info on this. My understanding is that the generated code has an exception now that it has the @Generated annotation.

@stuartmorgan-g
Copy link
Collaborator

We want generated code to be high quality, so we should evaluate lints in that light. Making the classes final seems like something that improves the quality of the code (and in particular, code that clients will be directly interacting with), so this seems like a good change regardless of any one specific client's needs.

@gaaclarke
Copy link
Member

We want generated code to be high quality, so we should evaluate lints in that light. Making the classes final seems like something that improves the quality of the code (and in particular, code that clients will be directly interacting with), so this seems like a good change regardless of any one specific client's needs.

High quality in terms of stability and performance? yes. Of a specific style? not particularly. We'll have to make that decision on a case by case decision. If we are in a position where the client doesn't care anymore it probably isn't worth prioritizing considering it is a breaking change. We don't want to get in a situation where we are responding to linter complaints since clients settings can differ between customers.

@stuartmorgan-g
Copy link
Collaborator

We'll have to make that decision on a case by case decision.

Yes, and that is exactly what I did in the comment you are replying to:

Making the classes final seems like something that improves the quality of the code (and in particular, code that clients will be directly interacting with), so this seems like a good change regardless of any one specific client's needs.

We don't want to get in a situation where we are responding to linter complaints since clients settings can differ between customers.

Do you think there are are people who have a linter setting that says that data classes shouldn't be final? I highly doubt that.

@tarrinneal
Copy link
Contributor Author

It seems to me that these changes are generally worth implementing. This doesn't mean we need to prioritize lint issues in the future, but this work is already done.

I think the classes should be final, and as such this change is worthwhile despite "breaking".

Copy link
Member

@gaaclarke gaaclarke left a comment

Choose a reason for hiding this comment

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

Sounds good to me since it's something we want beyond users' linter settings. There is value to this beyond clients' linter settings where subclassing these generated classes would be erroneous because their Dart counterpart would not match.

@tarrinneal tarrinneal added the autosubmit Merge PR when tree becomes green via auto submit App label Jan 19, 2023
@auto-submit auto-submit bot merged commit 69954f2 into flutter:main Jan 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

autosubmit Merge PR when tree becomes green via auto submit App p: pigeon

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[pigeon] Fix some straggler lint issues in the codegen

3 participants