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

refactor: helper method for register swagger api #12352

Conversation

deepto98
Copy link
Contributor

Description

Closes: #12149


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

@deepto98 deepto98 requested a review from a team as a code owner June 24, 2022 22:04
@deepto98 deepto98 force-pushed the deepto98/helper-method-for-RegisterSwaggerAPI branch 2 times, most recently from 0a3e883 to e6853f4 Compare June 24, 2022 22:18
@deepto98 deepto98 force-pushed the deepto98/helper-method-for-RegisterSwaggerAPI branch from e6853f4 to b42e3e9 Compare June 24, 2022 22:21
Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

I don't personally like the idea of Swagger existing in BaseApp. I'm sure we can move this somewhere else, perhaps the server package.

Also, let's be sure to get a changelog entry for this too.

@deepto98
Copy link
Contributor Author

deepto98 commented Jul 3, 2022

@alexanderbez I've moved the RegisterSwaggerAPI function to the server package, and added a changelog for the same.

@julienrbrt
Copy link
Member

Do we still need this go import in app.go then?

// unnamed import of statik for swagger UI support
_ "github.com/cosmos/cosmos-sdk/client/docs/statik"` 

Nit: Maybe the file name should just be swagger.go.

@deepto98
Copy link
Contributor Author

deepto98 commented Jul 4, 2022

@julienrbrt I've removed the import and changed the filename to swagger.go.

CHANGELOG.md Outdated Show resolved Hide resolved

"github.com/cosmos/cosmos-sdk/client"
"github.com/gorilla/mux"
"github.com/rakyll/statik/fs"
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need the _ "github.com/cosmos/cosmos-sdk/client/docs/statik" import?

Copy link
Contributor Author

@deepto98 deepto98 Jul 10, 2022

Choose a reason for hiding this comment

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

I think so, I removed it since you'd mentioned it here. Do you think its needed?

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant moving it back to another package. Have you tested this locally?

Copy link
Contributor

Choose a reason for hiding this comment

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

bump @deepto98 :)

server/swagger.go Outdated Show resolved Hide resolved
Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>
@julienrbrt julienrbrt added the backport/0.46.x PR scheduled for inclusion in the v0.46's next stable release label Jul 12, 2022
simapp/app.go Outdated Show resolved Hide resolved
abci "github.com/tendermint/tendermint/abci/types"
"github.com/tendermint/tendermint/libs/log"
tmos "github.com/tendermint/tendermint/libs/os"
dbm "github.com/tendermint/tm-db"

"github.com/cosmos/cosmos-sdk/depinject"
"github.com/cosmos/cosmos-sdk/server"
Copy link
Member

Choose a reason for hiding this comment

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

Can you group the imports :)

@codecov
Copy link

codecov bot commented Jul 21, 2022

Codecov Report

Merging #12352 (229a17a) into main (0b69859) will increase coverage by 0.03%.
The diff coverage is 22.22%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #12352      +/-   ##
==========================================
+ Coverage   65.16%   65.19%   +0.03%     
==========================================
  Files         692      693       +1     
  Lines       70837    70836       -1     
==========================================
+ Hits        46158    46179      +21     
+ Misses      22109    22086      -23     
- Partials     2570     2571       +1     
Impacted Files Coverage Δ
server/swagger.go 0.00% <0.00%> (ø)
simapp/app.go 81.08% <100.00%> (+4.79%) ⬆️
x/group/keeper/keeper.go 58.66% <0.00%> (+0.39%) ⬆️
crypto/keys/internal/ecdsa/privkey.go 83.01% <0.00%> (+1.88%) ⬆️
x/distribution/simulation/operations.go 90.42% <0.00%> (+9.57%) ⬆️

@julienrbrt
Copy link
Member

julienrbrt commented Jul 26, 2022

Hey @deepto98, this is almost ready to get merged 🙂. Can you implement the last two comments?

@deepto98
Copy link
Contributor Author

Okay, sure. Was a bit busy the past few days, will do these.

@julienrbrt
Copy link
Member

julienrbrt commented Jul 26, 2022

Okay, sure. Was a bit busy the past few days, will do these.

No worries, thank you!! 👍🏾

@tac0turtle
Copy link
Member

@julienrbrt want to take over this pr. we can close and reopen from a non fork

@tac0turtle
Copy link
Member

@alexanderbez or @facundomedica could you push the pr across the finish line

@alexanderbez
Copy link
Contributor

@alexanderbez or @facundomedica could you push the pr across the finish line

what is left? It seems @julienrbrt gave a thumps up so I think he's got it? If not, I can take a stab at it.

@julienrbrt
Copy link
Member

Replaced by #12955

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/0.46.x PR scheduled for inclusion in the v0.46's next stable release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make re-usable helper method for RegisterSwaggerAPI
6 participants