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.

Sign up for GitHub

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

Jump to bottom

Update to latest lints and Dart SDK #828

Merged
merged 8 commits into from
May 31, 2023
Merged

Update to latest lints and Dart SDK #828

merged 8 commits into from
May 31, 2023

Conversation

kevmoo
Copy link
Collaborator

@kevmoo kevmoo commented May 1, 2023

No description provided.

@kevmoo kevmoo requested a review from osa1 May 1, 2023 22:23
@kevmoo
Copy link
Collaborator Author

kevmoo commented May 3, 2023

@mraleph ?

@mraleph
Copy link
Member

mraleph commented May 3, 2023

I will let @osa1 review once he has cycles.

osa1
osa1 reviewed May 4, 2023
View reviewed changes
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.

Most of the changes LGTM, but I'm not sure if the changes in coded_buffer_writer are safe (added an inline comment).

Secondly, we should land #816 first or opt-out from internal lints in the internal version (which I think makes sense given the package is third-party) and revert the CLs listed in #816.

I have no cycles to keep these two versions in sync and this kind of style changes (either in the internal version or open source) only make it difficult for actual users to make changes. For example #825 is for an internal user and we have to do those changes manually in the internal version to be able to test and actually use it. #790 is another example of a useful change.

protobuf/lib/src/protobuf/coded_buffer_writer.dart Show resolved Hide resolved
@kevmoo kevmoo marked this pull request as draft May 4, 2023 22:24
@kevmoo kevmoo marked this pull request as ready for review May 27, 2023 02:01
@kevmoo
Copy link
Collaborator Author

kevmoo commented May 27, 2023

@osa1 – now ready for your review!

osa1
osa1 reviewed May 30, 2023
View reviewed changes
protoc_plugin/test/message_test.dart Show resolved Hide resolved
protoc_plugin/test/message_test.dart Show resolved Hide resolved
protobuf/test/list_test.dart Show resolved Hide resolved
@kevmoo
Copy link
Collaborator Author

kevmoo commented May 30, 2023

@osa1 – PTAL

@osa1 osa1 merged commit da6953c into master May 31, 2023
@osa1 osa1 deleted the lint_update branch May 31, 2023 09:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@osa1 osa1 osa1 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kevmoo @mraleph @osa1