search

go-openapi / spec

openapi specification object model
Apache License 2.0
389 stars 98 forks source link

Fix `ResolveParameterWithBase` bug (which is broken now) #128

Closed magodo closed 3 years ago

magodo commented 3 years ago

When the opts is set with RelativeBase set, current ResolveParameterWithBase will just fail. Referring to the implementation of ResolveWithBase, adding the missing part to make it work.

codecov[bot] commented 3 years ago

Codecov Report

Merging #128 (3e50065) into master (81c4553) will decrease coverage by 0.03%. The diff coverage is 25.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #128      +/-   ##
==========================================
- Coverage   58.42%   58.38%   -0.04%     
==========================================
  Files          25       25              
  Lines        1912     1915       +3     
==========================================
+ Hits         1117     1118       +1     
- Misses        615      616       +1     
- Partials      180      181       +1
Impacted Files Coverage Δ
expander.go 70.26% <25.00%> (-0.33%) :arrow_down:

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 81c4553...c10dee8. Read the comment docs.

fredbi commented 3 years ago

also if ResolveParameterWithBase is broken, so is probably ResolveResponseWithBase, which follows the same logic. But you didn't address that part.

magodo commented 3 years ago

@fredbi Thank you for the feedbacks! I have done the same change onto ResolveResponseWithBase, and added UT(s) for both. Please take another look!

fredbi commented 3 years ago

@magodo your PR looks good to me

Could you please rebase and PGP-sign your commit. Then I'll merge straight away.

Thank you for contributing!

magodo commented 3 years ago

@fredbi I have squashed and rebased the commit and signed off it. Thank your for reviewing :)

fredbi commented 3 years ago

@magodo we are almost good... I've reactived the linter in CI... (was disabled for sometime, as golangci ceased to operate).

Just need to address this:

expander_test.go:308:10: string `fixtures/expansion/crossFileRef.json` has 2 occurrences, make it a constant (goconst)

    path := "fixtures/expansion/crossFileRef.json"
fredbi commented 3 years ago

I've added #130 that should merge your PR automatically when applied

fredbi commented 3 years ago

@magodo I've merged your commit (with #130), with the linter issue fixed. Thank you so much for picking this one.