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 ValidateName to CompositeError #32

Merged
merged 2 commits into from
Aug 23, 2021
Merged

Add ValidateName to CompositeError #32

merged 2 commits into from
Aug 23, 2021

Conversation

veleek
Copy link
Contributor

@veleek veleek commented Jun 30, 2021

Added a ValidateName method to CompositeError to mirror the same API in Validation type to enable generated validation logic to alias CompositeErrors the same wait it aliases Validations.

Fixes #31

Added a ValidateName method to CompositeError to mirror the same API in
Validation type to enable generated validation logic to alias
CompositeErrors the same wait it aliases Validations.

Signed-off-by: Ben Randall <ben.randall@unity3d.com>
@veleek
Copy link
Contributor Author

veleek commented Jun 30, 2021

I took a closer look at this and I'm not sure that it'll do what I'm expecting it to do. I turns out that ValidateName only sets the name property if it's not already set. This seems to contradict the comment which is: // ValidateName produces an error message name for an aliased property.

My quick overview of some subset of the code we have that's generated using go-swagger will never pass "" as Validation.Name so ValidateName essentially does nothing. Is this expected?

@veleek veleek marked this pull request as draft June 30, 2021 16:51
@veleek
Copy link
Contributor Author

veleek commented Jun 30, 2021

Added a change showing how I would expect it to actually work. Note that the tests have not been updated at the moment so they'll still fail because they're written according to the existing behavior.

@veleek veleek requested a review from casualjim June 30, 2021 17:02
@veleek
Copy link
Contributor Author

veleek commented Jul 7, 2021

@casualjim - Where's the right place to discuss something like this to get sign-off?

@casualjim
Copy link
Member

This PR is still marked as draft, so I can't review it

@casualjim casualjim merged commit b4f29f3 into go-openapi:master Aug 23, 2021
@veleek
Copy link
Contributor Author

veleek commented Aug 24, 2021

Hey @casualjim - I see this was merged to unblock youyuanwu. I don't remember if I ever updated the tests to pass with this change. I was waiting for some feedback around if we thought this was a reasonable solution before fixing the tests but forgot to follow up with you to ask again.

I haven't tried running the tests in a while though, so I'm not aware what the current state is.

@casualjim
Copy link
Member

ic, we can still update the tests. I merged the change into go-swagger master so I had to bring this one along

@casualjim
Copy link
Member

One thought I had though is that we should just typecheck based on an interface instead of typeswitching but thats in the go-swagger repo

guillemj added a commit to guillemj/golang-go-openapi-errors that referenced this pull request Nov 19, 2021
This fixes the tests for the go-openapi#32 PR, which did not update them to match
the implementation changes.
@guillemj guillemj mentioned this pull request Nov 19, 2021
guillemj added a commit to guillemj/golang-go-openapi-errors that referenced this pull request Nov 19, 2021
This fixes the tests for the go-openapi#32 PR, which did not update them to match
the implementation changes.

Signed-off-by: Guillem Jover <gjover@sipwise.com>
guillemj added a commit to guillemj/golang-go-openapi-errors that referenced this pull request Nov 19, 2021
This fixes the tests for the go-openapi#32 PR, which did not update them to match
the implementation changes.

Signed-off-by: Guillem Jover <gjover@sipwise.com>
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.

Add support for ValidateName to CompositeError
2 participants