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

Remove duplicate consts #773

Merged
merged 5 commits into from
Nov 18, 2022
Merged

Conversation

devoncarew
Copy link
Collaborator

@devoncarew devoncarew commented Nov 17, 2022

A few more cleanups to the generated code:

  • remove unnecessary consts (and the ignore: unnecessary_const directive)
  • when we print out base64, don't emit lines longer than 80 chars
  • adjust import ordering

@devoncarew
Copy link
Collaborator Author

(part of #771)

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.

LGTM, but I think formatting is not working as expected. (see my inline comment)

'$_convertImportPrefix.base64Decode($base64Lines);');
}

List<String> splitString(String str, int segmentLength) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
List<String> splitString(String str, int segmentLength) {
static List<String> _splitString(String str, int segmentLength) {

We could also move this to a utility library, along with the stuff in string_escape.dart. We may also have other fairly general utility functions elsewhere in the package.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sgtm! (I may leave that as work for a future PR)

I in-lined your suggestion into my local edits

],
};

/// Descriptor for `PhoneType`. Decode as a `google.protobuf.EnumDescriptorProto`.
final $typed_data.Uint8List phoneTypeDescriptor = $convert.base64Decode('CglQaG9uZVR5cGUSCgoGTU9CSUxFEAASCAoESE9NRRABEggKBFdPUksQAhIMCghCVVNJTkVTUxAC');
final $typed_data.Uint8List phoneTypeDescriptor = $convert.base64Decode('CglQaG9uZVR5cGUSCgoGTU9CSUxFEAASCAoESE9NRRABEggKBFdPUksQAhIMCghCVVNJTkVTUx''AC');
Copy link
Member

Choose a reason for hiding this comment

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

This is longer than 80 characters, is this expected? Is this a problem with the formatter?

I think the intended formatting is something like

final $typed_data.Uint8List phoneTypeDescriptor = $convert.base64Decode(
  'CglQaG9uZVR5cGUSCgoGTU9CSUxFEAASCAoESE9NRRABEggKBFdPUksQAhIMCghCVVNJTkVTUx'
  'AC');

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, good catch.

I adjusted the output a bit so this will look better w/o formatting, but I think what we're seeing here is that these test/goldens/*.pbjson files aren't getting the formatter run on them after generation. (separately, I don't know why they don't end in a .dart extension; they do seem to contain dart code and not json)

@@ -685,12 +695,12 @@ const _fileIgnores = {
'annotate_overrides',
'camel_case_types',
'constant_identifier_names',
'deprecated_member_use',
'directives_ordering',
'library_prefixes',
'non_constant_identifier_names',
'prefer_final_fields',
'return_of_invalid_type',
Copy link
Member

Choose a reason for hiding this comment

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

Not related to this PR, but I think we can remove this ignore as well. I tried removing it locally and it doesn't generate any warnings in test protos.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll remove in this PR (I verified what you were saying locally as well)

Copy link
Collaborator Author

@devoncarew devoncarew left a comment

Choose a reason for hiding this comment

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

Updated the PR based on review comments -

'$_convertImportPrefix.base64Decode($base64Lines);');
}

List<String> splitString(String str, int segmentLength) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sgtm! (I may leave that as work for a future PR)

I in-lined your suggestion into my local edits

@@ -685,12 +695,12 @@ const _fileIgnores = {
'annotate_overrides',
'camel_case_types',
'constant_identifier_names',
'deprecated_member_use',
'directives_ordering',
'library_prefixes',
'non_constant_identifier_names',
'prefer_final_fields',
'return_of_invalid_type',
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll remove in this PR (I verified what you were saying locally as well)

],
};

/// Descriptor for `PhoneType`. Decode as a `google.protobuf.EnumDescriptorProto`.
final $typed_data.Uint8List phoneTypeDescriptor = $convert.base64Decode('CglQaG9uZVR5cGUSCgoGTU9CSUxFEAASCAoESE9NRRABEggKBFdPUksQAhIMCghCVVNJTkVTUxAC');
final $typed_data.Uint8List phoneTypeDescriptor = $convert.base64Decode('CglQaG9uZVR5cGUSCgoGTU9CSUxFEAASCAoESE9NRRABEggKBFdPUksQAhIMCghCVVNJTkVTUx''AC');
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, good catch.

I adjusted the output a bit so this will look better w/o formatting, but I think what we're seeing here is that these test/goldens/*.pbjson files aren't getting the formatter run on them after generation. (separately, I don't know why they don't end in a .dart extension; they do seem to contain dart code and not json)

@osa1 osa1 merged commit ed68426 into google:master Nov 18, 2022
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.

2 participants