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

replace github.com/go-resty/resty with gopkg.in/resty.v1 #770

Closed
wants to merge 3 commits into from

Conversation

dmitris
Copy link

@dmitris dmitris commented Oct 8, 2018

@johanbrandhorst - as discussed on Slack, did not update generated files

@@ -16,7 +16,7 @@ required = [
[[constraint]]
# Also defined in WORKSPACE
revision = "f8815663de1e64d57cdd4ee9e2b2fa96977a030e"
name = "github.com/go-resty/resty"
name = "gopkg.in/resty.v1"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this still work? I think since our generated files use the other import we still need to include both. I'm not sure this is a complete fix. The correct way to fix this would be to correct the generated files as well. Sorry I know that means it will be harder to get done.

@johanbrandhorst
Copy link
Collaborator

I don't think we can merge this as is - dep will still add the old dependency to the lockfile since some of the checked in files import it. We need this to be solved in the generator first (or run a post-generate script to replace the import ourselves...). Have you raised an issue with swagger-codegen?

@dmitris
Copy link
Author

dmitris commented Oct 8, 2018

related (for reference) - go-resty/resty#178

also https://github.com/go-resty/resty/blob/master/go.mod#L1 contains module gopkg.in/resty.v1

@johanbrandhorst
Copy link
Collaborator

Did you regenerate these files or just search replace?

@dmitris
Copy link
Author

dmitris commented Oct 10, 2018

@johanbrandhorst - search-replaced in the branch for now

opened swagger-api/swagger-codegen#8798 - please add the details as needed.

It would be great if we could move to using the latest version of swagger-codegen (2.3.1) as mentioned in #254. Currently when I try to generate code with swagger-codegen 2.3.1, I'm getting invalid Go generated:

./examplepb_a_bit_of_everything.go:48:13: undefined: ExamplepbNumericEnum
./examplepb_a_bit_of_everything.go:50:17: undefined: PathenumPathEnum
./examplepb_a_bit_of_everything.go:68:22: undefined: ExamplepbNumericEnum
./examplepb_a_bit_of_everything.go:78:22: undefined: ExamplepbNumericEnum

If I understand it correctly, `a_bit_of_everything.swagger.json has definitions for the lower-case fields:

    "examplepbNumericEnum": {
      "type": "string",
      "enum": [
        "ZERO",
        "ONE"
      ],
      "default": "ZERO",
      "description": "NumericEnum is one or zero.\n\n - ZERO: ZERO means 0\n - ONE: ONE means 1"
    },
    "pathenumPathEnum": {
      "type": "string",
      "enum": [
        "ABC",
        "DEF"
      ],
      "default": "ABC"
    },

which becomes in examplepb_numeric_enum.go:

// ExamplepbNumericEnum : NumericEnum is one or zero.   - ZERO: ZERO means 0  - ONE: ONE means 1
type examplepbNumericEnum string

and contradicts usage as in the following enum:

// ExamplepbNumericEnum : NumericEnum is one or zero.   - ZERO: ZERO means 0  - ONE: ONE means 1
type examplepbNumericEnum string

// List of examplepbNumericEnum
const (
	ZERO ExamplepbNumericEnum = "ZERO"
	ONE  ExamplepbNumericEnum = "ONE"
)

@johanbrandhorst
Copy link
Collaborator

I think we're more likely just to drop this dependency than update the version. I reiterate though - this will only happen after #772 has been merged.

@johanbrandhorst
Copy link
Collaborator

I'm gonna close this, lets resume the discussion in #254

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