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

Add embed message field option #56

Merged
merged 1 commit into from
Sep 23, 2021
Merged

Add embed message field option #56

merged 1 commit into from
Sep 23, 2021

Conversation

Adphi
Copy link
Contributor

@Adphi Adphi commented Jun 25, 2021

[UPDATED after comments] add embedding support with (go.field).embed.
Example:

message MessageWithEmbeddedFields {
  Embedded message = 1 [(go.field).embed = true];
}

message Embedded {
  string message = 1;
}

@ydnar
Copy link
Contributor

ydnar commented Jun 25, 2021

Wow, interesting. Some quick feedback, as I'm on my phone:

  • I'd prefer the API to be explicit rather than implicit based on an empty name, e.g.: go.field.embedded = true
  • Can you revert the formatting changes in the proto files that aren't relevant to this PR?
  • Tests should exist and pass.

Thanks for the PR!

@Adphi
Copy link
Contributor Author

Adphi commented Jun 26, 2021

I agree with you on explicit over implicit (I didn't want to change the defined API).
I added the go.field.embedded option.
I will revert formatting changes.

@Adphi Adphi changed the title allow embedded message field with empty name Add embedded message field option Jun 26, 2021
@Adphi
Copy link
Contributor Author

Adphi commented Jun 26, 2021

@ydnar do you want to explicitly fail on unsupported options annotation, like oneof field annotated as embedded or non message type annotated as embedded or just silently ignore these unsupported options?

@ydnar
Copy link
Contributor

ydnar commented Jun 26, 2021

@ydnar do you want to explicitly fail on unsupported options annotation, like oneof field annotated as embedded or non message type annotated as embedded or just silently ignore these unsupported options?

Explicitly fail with a verbose error message.

@Adphi
Copy link
Contributor Author

Adphi commented Jun 28, 2021

@ydnar do you want to explicitly fail on unsupported options annotation, like oneof field annotated as embedded or non message type annotated as embedded or just silently ignore these unsupported options?

Explicitly fail with a verbose error message.

I agree with you, but I can see some limitations based on the existing API...
None of the scan methods returns an error, which does not allow validation of protobuf options, this would lead to an ugly os.Exit(code) inside one of the scan methods, the logger output being redirected to io.Discard by default if PROTO_PATCH_DEBUG_LOGGING is not defined, the log.Fatal message would not reach the user... I think it might be better to address the problem in a separated issue / pull request.

@Adphi Adphi force-pushed the embedded branch 2 times, most recently from ffa4674 to 65b835e Compare June 28, 2021 18:26
@Adphi
Copy link
Contributor Author

Adphi commented Jul 3, 2021

Ping @ydnar

@ydnar
Copy link
Contributor

ydnar commented Jul 3, 2021

Ping @ydnar

Hey there—we’re on vacation for the July 4th weekend. I’ll take a look at the PR next week when we’re back home!

Copy link
Contributor

@ydnar ydnar left a comment

Choose a reason for hiding this comment

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

Hi there, thanks for your patience. I’d love to see this merged with a couple changes.

patch/patcher.go Outdated Show resolved Hide resolved
patch/patcher.go Outdated Show resolved Hide resolved
patch/patcher.go Outdated Show resolved Hide resolved
patch/patcher.go Outdated Show resolved Hide resolved
patch/go.proto Outdated Show resolved Hide resolved
@Adphi
Copy link
Contributor Author

Adphi commented Sep 20, 2021

Hi @ydnar !
Thanks for your reply. I'll make the requested changes as soon as I can.
FYI: I started to work on the "casttype" implementation 😃

@ydnar
Copy link
Contributor

ydnar commented Sep 20, 2021

FYI: I started to work on the "casttype" implementation 😃

Wow, I’m curious to see how that works.

@Adphi Adphi requested a review from ydnar September 20, 2021 16:23
@Adphi Adphi changed the title Add embedded message field option Add embed message field option Sep 21, 2021
patch/go.proto Outdated Show resolved Hide resolved
patch/patcher.go Outdated Show resolved Hide resolved
patch/patcher.go Outdated Show resolved Hide resolved
Copy link
Contributor

@ydnar ydnar left a comment

Choose a reason for hiding this comment

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

Additional comments.

@Adphi Adphi force-pushed the embedded branch 2 times, most recently from 7a43c1e to 8c88795 Compare September 22, 2021 13:31
@Adphi Adphi requested a review from ydnar September 22, 2021 13:33
tests/lint/lint_test.go Outdated Show resolved Hide resolved
tests/lint/lint_test.go Outdated Show resolved Hide resolved
tests/lint/lint_test.go Outdated Show resolved Hide resolved
tests/message/message_test.go Show resolved Hide resolved
@Adphi Adphi requested a review from ydnar September 22, 2021 19:05
@ydnar ydnar merged commit 9ccb82d into alta:main Sep 23, 2021
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