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.

Sign up for GitHub

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

Jump to bottom

refactor: remove dependency on gin #800

Merged
merged 5 commits into from
Nov 22, 2020
Merged

refactor: remove dependency on gin #800

merged 5 commits into from
Nov 22, 2020

Conversation

akojo
Copy link
Contributor

@akojo akojo commented Oct 3, 2020

Describe the PR
Since swag doesn't have a direct dependency on gin-gonic/gin, drop it as a dependency in the main module:

  • Add go.mod into examples so that they can be used stand-alone and pull in correct deps
  • Use http package for handlers instead of gin in tests
  • Use http in example/basic since tests depend on in
  • Clean up main go.mod

Relation issue
#796

@codecov
Copy link

codecov bot commented Oct 3, 2020 •
edited
Loading

Codecov Report

Merging #800 (0301453) into master (c9056f0) will increase coverage by 1.06%.
The diff coverage is 91.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #800      +/-   ##
==========================================
+ Coverage   83.82%   84.88%   +1.06%     
==========================================
  Files           8        8              
  Lines        1663     1654       -9     
==========================================
+ Hits         1394     1404      +10     
+ Misses        159      145      -14     
+ Partials      110      105       -5
Impacted Files Coverage Δ
parser.go 82.01% <91.66%> (+2.16%) ⬆️

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 c9056f0...0301453. Read the comment docs.

@akojo akojo force-pushed the dont-depend-on-gin branch from bc69b26 to 343aeea Compare October 5, 2020 05:12
@easonlin404
Copy link
Member

@akojo could you help rebase master and fix conflicts?

@akojo akojo force-pushed the dont-depend-on-gin branch from 343aeea to ed86507 Compare October 5, 2020 17:18
@akojo
Copy link
Contributor Author

akojo commented Oct 5, 2020

@akojo could you help rebase master and fix conflicts?

Ah, again master ran away 😃 . Rebased.

@touyu
Copy link

touyu commented Oct 16, 2020 •
edited
Loading

Is this PR still working?

I'm using echo-swagger, I'm having trouble with swaggo/gin-swagger#128 because swag depends on gin-swagger.

@akojo
Copy link
Contributor Author

akojo commented Oct 28, 2020

Is there something I could help with to get this moving?

@ubogdan
Copy link
Contributor

ubogdan commented Oct 29, 2020

@akojo in order to review your PR we need to see CI tests passing.

@akojo akojo force-pushed the dont-depend-on-gin branch from 11d0ec7 to cae0df4 Compare November 5, 2020 10:49
@akojo
Copy link
Contributor Author

akojo commented Nov 5, 2020

Hmm, any idea why go1.13 tests fail? Running that specific test doesn't work on my fork since the paths don't match

@akojo akojo force-pushed the dont-depend-on-gin branch from cae0df4 to 2b234b3 Compare November 13, 2020 09:36
@akojo
Copy link
Contributor Author

akojo commented Nov 13, 2020

Heh, silly me. Didn't realize that my changes to examples/ broke the build

@akojo akojo force-pushed the dont-depend-on-gin branch from 2b234b3 to 46d26b6 Compare November 13, 2020 11:15
@ubogdan
Copy link
Contributor

ubogdan commented Nov 13, 2020 •
edited
Loading

For some reason, the coverage decreased by 0.25 and the checks are failing.
@easonlin404 I can't merge the PR due to this issue.

@akojo
Copy link
Contributor Author

akojo commented Nov 18, 2020

🤔 Looks like updating github.com/go-openapi/spec to v0.19.14 breaks the build by changing the formatting of output slightly

@akojo
Copy link
Contributor Author

akojo commented Nov 18, 2020

See #835

@akojo akojo force-pushed the dont-depend-on-gin branch from 23002ef to 0573df1 Compare November 19, 2020 16:20
sdghchj
sdghchj previously approved these changes Nov 19, 2020
View reviewed changes
Copy link
Member

@sdghchj sdghchj left a comment

Choose a reason for hiding this comment

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

LGTM

@akojo
Copy link
Contributor Author

akojo commented Nov 20, 2020

That -0.13% coverage decrease seems to be bogus since parser test should never hit the line in question in the first place. Only duplicate @ID declaration in tests is in a POST annotation 🤔

akojo added 3 commits November 20, 2020 18:15
@akojo
Remove dependency on gin from tests
b11c96c 
Since tests only test parsing of go source and swaggo special comments,
an advanced router is not really necessary
@akojo
Clean up modules
b5c6d4b
@akojo
Add inner package to testdata/nested2
e662cc6 
Since testdata/nested doesn't point to outside packages anymore, this
caused nested parsing test to skip exercising one if-branch. Add more
nesting to nested2 to regain that.
@akojo akojo force-pushed the dont-depend-on-gin branch from 0573df1 to e662cc6 Compare November 20, 2020 16:19
akojo added 2 commits November 21, 2020 19:18
@akojo
Refactor Parser.checkOperationIDUniqueness
bfd5e5d 
Remove duplication in code that checks for duplicate operation ids
@akojo
Better duplicate id detection test coverage
0301453 
Also exercise rest of HTTP methods when testing for duplicate @id
detection
@akojo akojo force-pushed the dont-depend-on-gin branch from 748de64 to 0301453 Compare November 22, 2020 10:28
sdghchj
sdghchj approved these changes Nov 22, 2020
View reviewed changes
Copy link
Member

@sdghchj sdghchj left a comment

Choose a reason for hiding this comment

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

LGTM

@sdghchj sdghchj merged commit 3d083d3 into swaggo:master Nov 22, 2020
@akojo akojo deleted the dont-depend-on-gin branch November 22, 2020 16:56
This was referenced Nov 23, 2020
Closed
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sdghchj sdghchj sdghchj approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@akojo @easonlin404 @touyu @ubogdan @sdghchj