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 support for proteus:",(protobuf field id)" in golang tag #109

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

liasece
Copy link

@liasece liasece commented Mar 1, 2019

Now you can specify the ID of the peoto file in the following way:

type Example struct {
    Filed int32 \`proteus:",101"\`
}

This example will generate the following protobuf message.

message Example {
        int32 Filed = 101;
}

Copy link
Contributor

@dennwc dennwc left a comment

Choose a reason for hiding this comment

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

CI failures seems unrelated, let me fix it first.

The change looks good in general, bu please add some tests to check if it works as intended.

scanner/package.go Show resolved Hide resolved
if len(tags) == 0 {
return 0
}
i, err := strconv.Atoi(tags[0])
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it may be better to use proteus:",101" notation - it will be easier to extend later.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for your suggestion, as you wish.

@dennwc
Copy link
Contributor

dennwc commented Mar 1, 2019

Please rebase on the latest master - it should fix the CI.

Thanks to dennwc for advice it may be better to use proteus:",101" notation - it will be easier to extend later.
@liasece liasece force-pushed the master branch 2 times, most recently from 694cddc to 6204512 Compare March 2, 2019 03:39
Signed-off-by: Denys Smirnov <[email protected]>
@liasece liasece changed the title add support for proteus:"(protobuf field id)" in golang tag add support for proteus:",(protobuf field id)" in golang tag Mar 2, 2019
add test in transform_test and scanner_test.
add an example of use.
@liasece
Copy link
Author

liasece commented Mar 2, 2019

Add tests and example

Copy link
Contributor

@dennwc dennwc left a comment

Choose a reason for hiding this comment

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

Looks good now, but need to regenerate files.

Also please sign your commits as described here (DCO):
https://github.com/src-d/guide/blob/master/engineering/licensing.md#developer-certificate-of-origin

// Field3 int

// 4th field, use `proteus:",4"` forward compatible
Field4 int `proteus:",4"`
Copy link
Contributor

Choose a reason for hiding this comment

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

You should also update the generated files in the example folder - this field will affect some of those files.

@@ -5,4 +5,9 @@ package categories
type CategoryOptions struct {
ShowPrices bool
CanBuy bool
// The next field was deleted.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// The next field was deleted.
// The next field was added initially (with a number 3) and was deprecated later.

// The next field was deleted.
// Field3 int

// 4th field, use `proteus:",4"` forward compatible
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// 4th field, use `proteus:",4"` forward compatible
// Field4 uses `proteus:",4"` for forward compatibility, see Field3 comment above.

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