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

Modularize and add validation #22

Merged
merged 28 commits into from
Feb 25, 2019
Merged

Modularize and add validation #22

merged 28 commits into from
Feb 25, 2019

Conversation

rvolosatovs
Copy link
Contributor

@rvolosatovs rvolosatovs commented Feb 22, 2019

Summary:

Closes #21
Closes #18
References TheThingsNetwork/lorawan-stack#51

Changes:

Notes for Reviewers:

This PR uses forks of both https://github.com/lyft/protoc-gen-star (it uses features in lyft/protoc-gen-star#51) and https://github.com/lyft/protoc-gen-validate(minor changes there to improve GoGo support and use fieldpaths)

I forked protoc-gen-validate, since only minor changes are necessary to make it work with fieldpaths(TheThingsIndustries/protoc-gen-validate@0e040cd), however we do not need to maintain any validators and we're fully compatible with the upstream plugin.

cc @KrishnaIyer

@rvolosatovs rvolosatovs added this to the February 2019 milestone Feb 22, 2019
@rvolosatovs rvolosatovs self-assigned this Feb 22, 2019
Copy link
Contributor

@htdvisser htdvisser left a comment

Choose a reason for hiding this comment

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

Generally looks fine to me.

This PR does multiple things at the same time. Please try to scope PRs to a single thing in the future.

Some more comments would be welcome too. You'll make the lives your future self, the rest of the team and open source contributors a lot easier.

.travis.yml Outdated Show resolved Hide resolved
build: extensions
CGO_ENABLED=0 go build -ldflags "-w -s" -o dist/protoc-gen-fieldmask-$(shell go env GOOS)-$(shell go env GOARCH)$(shell go env GOEXE) .
build:
CGO_ENABLED=0 go build -ldflags "-w -s" -o dist/protoc-gen-fieldmask .
Copy link
Contributor

Choose a reason for hiding this comment

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

I would keep the suffix

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's unnecessary complexity.
We're going to make releases with goreleaser if we need them.
This make target is only useful for development and in that case the GOOS and GOARCH are always known by the caller.

README.md Show resolved Hide resolved
@rvolosatovs
Copy link
Contributor Author

rvolosatovs commented Feb 22, 2019

Here's how the generated code looks like for lorawan-stack: rvolosatovs/lorawan-stack@9d36c19
It's a bit rough around the edges, but good enough for MVP
Note, that there still is a little issue with CustomName handling in oneofs, which I will have to fix in https://github.com/TheThingsIndustries/protoc-gen-star before this can be merged. I will add a testcase for that too.

@rvolosatovs rvolosatovs force-pushed the feature/protoc-gen-star branch 3 times, most recently from ae22044 to c790ac2 Compare February 25, 2019 13:06
@rvolosatovs rvolosatovs force-pushed the feature/protoc-gen-star branch from c790ac2 to 8f13f5e Compare February 25, 2019 13:23
@rvolosatovs rvolosatovs force-pushed the feature/protoc-gen-star branch 2 times, most recently from 4ea3051 to 08e7415 Compare February 25, 2019 13:47
@rvolosatovs rvolosatovs force-pushed the feature/protoc-gen-star branch 3 times, most recently from 71fb4ce to 2d5c8f7 Compare February 25, 2019 14:08
Copy link

@KrishnaIyer KrishnaIyer left a comment

Choose a reason for hiding this comment

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

Code looks quite good. Also checked the generated validators.

I have a question with the error handling in the code generated by protoc-gen-validate.

			if !_ApplicationIdentifiers_ApplicationID_Pattern.MatchString(m.GetApplicationID()) {
				return ApplicationIdentifiersValidationError{
					field:  "ApplicationID",
					reason: "value does not match regex pattern \"^[a-z0-9](?:[-]?[a-z0-9]){2,}$\"",
				}
			}

Two questions:

  1. Does this output the Go generated fieldname and not the proto name (ex: application_id)?
    That is undesirable since we want to return the proto names to the caller.
  2. Does it output the full path of the error(ex: end_device_ids.application_ids.application_id)?

main.go Outdated Show resolved Hide resolved
@rvolosatovs rvolosatovs force-pushed the feature/protoc-gen-star branch from 2d5c8f7 to 8eec7ff Compare February 25, 2019 19:59
@rvolosatovs
Copy link
Contributor Author

  1. Fixed that in https://github.com/TheThingsIndustries/protoc-gen-validate
  2. We do get the full path when error occurs: invalid Test.a: embedded message failed validation | caused by: invalid Test_TestNested.a: embedded message failed validation | caused by: invalid Test_TestNested_TestNestedNested.a: value must be inside range (24, 42]
    We may want to use the proto names for types here also, but that's not a priority now - please create an issue at https://github.com/TheThingsIndustries/protoc-gen-validate if this is required

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants