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

Handle duplicate route declaration better #968

Merged
merged 4 commits into from
Aug 11, 2021

Conversation

rubensayshi
Copy link
Contributor

Fixes issue when you accidentally have @Router /some/route declared twice.

Previously this would cause either of the 2 definitions to be used and it leaves the user very confused.

Tbh I'd personally just prefer the hard error when this mistake is made, but I think for backwards compatibility it might be nice to either have it warn by default (as it is now in this PR) or have the option to turn it into a warning, so I added the Strict option flag.
I think this could also be useful for more checks in the future?

Additionally I made RangeFiles sort the files to make sure it happens in deterministic order, prior to my fix to warn / error when there's a duplicate route and without this you'd randomly get one of the 2 routes and that really confuses the hell of you because instead of thinking you made a mistake it just looks like a very weird bug.

The "fix issue with files being parsed twice" was something that came up when I added the warning about duplicate routes and I noticed that in TestGen_cgoImports it was triggering as well, this was because it was parsing simple_cgo/api from both the searchDirs and from the ParseDependency block.

If you'd like me to split the PR into multiple then please let me know, or if you want it without the Strict config option for example

@codecov
Copy link

codecov bot commented Aug 9, 2021

Codecov Report

Merging #968 (e32aef6) into master (cb2bdb6) will increase coverage by 0.15%.
The diff coverage is 93.93%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #968      +/-   ##
==========================================
+ Coverage   87.86%   88.01%   +0.15%     
==========================================
  Files           8        8              
  Lines        1829     1869      +40     
==========================================
+ Hits         1607     1645      +38     
- Misses        127      128       +1     
- Partials       95       96       +1     
Impacted Files Coverage Δ
packages.go 76.69% <81.81%> (+2.31%) ⬆️
gen/gen.go 95.55% <100.00%> (+0.03%) ⬆️
parser.go 86.51% <100.00%> (+0.18%) ⬆️

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 cb2bdb6...e32aef6. Read the comment docs.

@sdghchj
Copy link
Member

sdghchj commented Aug 11, 2021

Thank you for your efforts.
If strict is set, it is necessary to sort the files, but whether it is still necessary to sort the files when strict is not set, in consideration of higher performance.

@rubensayshi
Copy link
Contributor Author

if you want I can change that, but I'd argue that the sorting is so lightweight that it's not worth it

@sdghchj
Copy link
Member

sdghchj commented Aug 11, 2021

if you want I can change that, but I'd argue that the sorting is so lightweight that it's not worth it

That depends the scale of a project I think.
Don't bother however.
Leave someone in future to optimize it if needed.

@sdghchj sdghchj merged commit 505d4f1 into swaggo:master Aug 11, 2021
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.

2 participants