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

feat: support global type overrides #1059

Merged
merged 5 commits into from
Nov 30, 2021
Merged

feat: support global type overrides #1059

merged 5 commits into from
Nov 30, 2021

Conversation

ian-fox
Copy link
Contributor

@ian-fox ian-fox commented Nov 26, 2021

Describe the PR
When using generated files (such as from sqlboiler) we encountered a situation where we wanted to override a type (e.g. have null.String, a struct with custom json marshal/unmarshalers, show up in the swagger docs as string instead of as a struct with a string and a bool) but could not use the swaggertype struct tag overrides, as sqlboiler does not allow you to customize those on a per-type basis.

This PR adds a global "overrides file" where you can specify a mapping of what type is actually used in the code, and what type (basic or otherwise) you'd like it to render as in the generated docs.

Please let me know if you think there are any changes that should be made!

Relation issue
#985 seems related

Additional context
The solution to our problem seemed to be either modifying sqlboiler to allow it to insert custom tags on a per-type basis (e.g. add a swaggertype:"string" tag whenever it generated a struct with a null.String) or to modify swaggo to be able to do replacements without the struct tag; this seemed like it was probably more broadly useful because it may help with other generators than just sqlboiler.

@ubogdan
Copy link
Contributor

ubogdan commented Nov 26, 2021

@ian-fox, is there any open issue regarding this issue? I'm asking this because we appreciate everyone's effort to write a PR, but sometimes it is safer to validate the solution before writing the implementation.

@ian-fox
Copy link
Contributor Author

ian-fox commented Nov 26, 2021

There is not, I figured it was small enough that is write it up, learn something in the process, and if it wasn't accepted then that's fine because it didn't take too long anyway.

If there are any changes you'd suggest I'm happy to make them, and if it's just a poor design that you'd rather not accept that's also totally fine.

@ubogdan
Copy link
Contributor

ubogdan commented Nov 28, 2021

@ian-fox the ability to replace the definitions without touching the source code sound good to me.

Somehow an attempt to solve this issue was partially solved by introducing "swaggerype" a long time ago.

I'm thinking it would be nice to have a simple dot file in the same way git has".gitingore". YAML/TOML is too heavy (the libraries are not light at all and it will introduce additional unwanted dependencies) and JSON is not so user-friendly when it comes to editing.

Eventually, we can add at a later time the ability to control the flow like cmd params do, without the need to update the CI pipeline.

Example swag dot file (.swag)

// replace a defintion 
replace database/sql.NullInt64 int64

// skip parsing a package
skip github.com/grpc/grpc-go

@ian-fox
Copy link
Contributor Author

ian-fox commented Nov 29, 2021

Sounds good to me! I'll go add those changes 😄

@ian-fox
Copy link
Contributor Author

ian-fox commented Nov 29, 2021

I think all the issues with the tests and static analysis should be fixed now.

README.md Show resolved Hide resolved
gen/gen.go Outdated Show resolved Hide resolved
gen/gen.go Show resolved Hide resolved
gen/gen.go Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Nov 29, 2021

Codecov Report

Merging #1059 (b9ffdec) into master (93acd2a) will decrease coverage by 0.20%.
The diff coverage is 87.50%.

❗ Current head b9ffdec differs from pull request most recent head b0fd3aa. Consider uploading reports for the commit b0fd3aa to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1059      +/-   ##
==========================================
- Coverage   94.42%   94.22%   -0.21%     
==========================================
  Files           9        9              
  Lines        2261     2320      +59     
==========================================
+ Hits         2135     2186      +51     
- Misses         66       70       +4     
- Partials       60       64       +4     
Impacted Files Coverage Δ
gen/gen.go 92.04% <78.94%> (-3.64%) ⬇️
operation.go 95.90% <100.00%> (+0.02%) ⬆️
parser.go 92.24% <100.00%> (+0.15%) ⬆️
types.go 100.00% <100.00%> (ø)

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 93acd2a...b0fd3aa. Read the comment docs.

@ubogdan
Copy link
Contributor

ubogdan commented Nov 29, 2021

I'm thinking it's easier to finish the code changes first and after that, you can take care of the code coverage.

@ubogdan
Copy link
Contributor

ubogdan commented Nov 30, 2021

Can you please fix the failing tests?

@ian-fox
Copy link
Contributor Author

ian-fox commented Nov 30, 2021

Yep. Sorry about that, was counting on CI to run the monkey-patch related ones because monkey does not work on my computer.

@ubogdan
Copy link
Contributor

ubogdan commented Nov 30, 2021

We need to keep compatible with older GO versions.

gen/gen_test.go:7:2: package io/fs is not in GOROOT (/opt/hostedtoolcache/go/1.15.15/x64/src/io/fs)

try simply return errors.New("file does not exist");

Copy link
Contributor

@ubogdan ubogdan left a comment

Choose a reason for hiding this comment

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

LGTM

@ubogdan ubogdan merged commit a7065ad into swaggo:master Nov 30, 2021
@ian-fox
Copy link
Contributor Author

ian-fox commented Nov 30, 2021

Thank you for all your help!

@ubogdan
Copy link
Contributor

ubogdan commented Nov 30, 2021

@ian-fox Thanks for your contribution.

You're wellcome.

@ian-fox ian-fox deleted the global-override branch December 1, 2021 09:22
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.

3 participants