Skip to content

swagger/fix: []string are not enum by swagger definition #7916

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

Merged
merged 5 commits into from
Aug 26, 2019

Conversation

sapk
Copy link
Member

@sapk sapk commented Aug 19, 2019

[]string are not enum by swagger definition this led the generation of a client to think that repo.code,repo.issues,repo.ext_issues,repo.wiki,repo.pulls,repo.releases,repo.ext_wiki is an array of []string ([][]string).
By definition, enum are one of an array like Permission that can take one value from none,read,write,admin,owner.
This PR change the definition to an example to be able to generate a valid client from the swagger specs. Currently the generated client doesn't compile because it try convert the []string enum to [][]string

@codecov-io
Copy link

codecov-io commented Aug 19, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@042089f). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #7916   +/-   ##
=========================================
  Coverage          ?   41.59%           
=========================================
  Files             ?      479           
  Lines             ?    64124           
  Branches          ?        0           
=========================================
  Hits              ?    26672           
  Misses            ?    33991           
  Partials          ?     3461

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 042089f...fbc8eaf. Read the comment docs.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 19, 2019
@sapk
Copy link
Member Author

sapk commented Aug 19, 2019

Somewhat related (at least for the generate limitation part) : go-swagger/go-swagger#1942

@lunny lunny added the type/docs This PR mainly updates/creates documentation label Aug 20, 2019
@guillep2k
Copy link
Member

image

Is it OK that units and permission have different representations?

@sapk
Copy link
Member Author

sapk commented Aug 23, 2019

Yes because permission only take one of the value (enum type) and units is a list (array) of manyof the value wich is not really an enum.

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Aug 23, 2019
@sapk
Copy link
Member Author

sapk commented Aug 23, 2019

This could maybe improved by defining an Go enum type for units and declare that units is an array of this Type. I have try but I doesn't have anythings fully working and it need a lot of refactor to be done on other parts of code. There also seems to be issue whit that on go-swagger so I used the example fields.

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Aug 26, 2019
@techknowlogick techknowlogick added this to the 1.10.0 milestone Aug 26, 2019
@techknowlogick techknowlogick added the modifies/api This PR adds API routes or modifies them label Aug 26, 2019
@sapk sapk merged commit 954fe0e into go-gitea:master Aug 26, 2019
@sapk sapk deleted the fix-swagger branch August 26, 2019 18:13
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/api This PR adds API routes or modifies them type/docs This PR mainly updates/creates documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants