[CO-327] Rely on Example type-class for Swagger API schema #3215
Conversation
Anviking
requested review from
adinapoli-iohk,
denisshevchenko and
parsonsmatt
as code owners
|
Hm, CI is failing, but it seems to only be warnings I didn't see in ghci. I will look into this. |
Anviking
changed the title
| import Control.Lens (At, Index, IxValue, at, ix, makePrisms, to, (?~)) | ||
| import Data.Aeson | ||
| import Data.Aeson.TH as A | ||
| import Data.Aeson.Types (toJSONKeyText, typeMismatch) | ||
| import qualified Data.Char as C | ||
| import Data.Swagger as S | ||
| import Data.Swagger as S hiding (Example, example) | ||
| import qualified Data.Swagger as SE (example) |
There was a problem hiding this comment.
We can keep S instead of SE here. They refer to the same module anyway :)
There was a problem hiding this comment.
Actually, there was a reason. This way I could simply copy-paste the Example-instances and have example refer to Swagger.Example.example, and accessing Data.Swagger.example as SE.example.
So we can, but have to make 50+ references to Example and example qualified. Feels a little bit wrong, but I guess explicitly is good?
There was a problem hiding this comment.
Sorry for the delay, but what I meant was simply to have:
import Data.Swagger hiding (Example, example)
import qualified Data.Swagger as SEven though, I'd rather list everything we use from Data.Swagger if it's realistic.
There was a problem hiding this comment.
Seems borderline unrealistic (2k LoC and lots of things from Data.Swagger). Should I spend time on it? Could perhaps get importify working.
There was a problem hiding this comment.
("unrealistic" as in I got to like 10-20 symbols and then gave up, and not sure if there are 25 in total or 100)
|
|
||
| instance Example WalletStateSnapshot | ||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
What about removing this whole file, moving the remaining instances in places where they belong 🤔 ?
There was a problem hiding this comment.
@KtorZ The remaining ones are
WalletSnapshotfrom ../wallet/src/Pos/Wallet/Web/Methods/Misc.hsCUpdateInfofrom ../wallet/src/Pos/Wallet/Web/ClientTypes/Types.hsMnemonicfrom ../wallet/src/Pos/Util/Mnemonic
Can we really depend on wallet-new from the old wallet?
Then, also, the class itself, and instances for (), NonEmpty, [], Maybe and Map need to be somewhere.
So it would seem difficult…?
There was a problem hiding this comment.
Hmmmm... Fair point. Not sure why we have this Example in the wallet-new in a first place since it's quite an isolated module. But it doesn't make sense to import anything from the wallet in the old one,
KtorZ
changed the title
1. Import and use `Example` typeclass in `../V1/Types.hs` instead of `Arbitrary` 2. Resolve circular dependencies `Errors.hs` -> `Types.hs` -> `Example.hs` -> `Response.hs` -> `Errors.hs` by moving `Example` instances from `Example.hs` to where the datatypes are defined, so that `Example.hs` does not need to import `Response.hs` or `Types.hs`.
|
Weird failure on appveyor, merging into the feature branch anyway. This doesn't seem related to the PR itself 👍 |
…e-typeclass [CO-327] Rely on Example type-class for Swagger API schema
…e-typeclass [CO-327] Rely on Example type-class for Swagger API schema
Description
Use
Exampletypeclass instead ofArbitrarywhen creating Swagger schema.Exampletypeclass in../V1/Types.hsinstead of
ArbitraryErrors.hs->Types.hs->Example.hs->Response.hs->Errors.hsby moving
Exampleinstances fromExample.hsto where the datatypesare defined, so that
Example.hsdoes not need to importResponse.hsor
Types.hs.Regarding name collisions between
Data.SwaggerandCardano.Wallet.API.V1.Swagger.Example, I suspecthidingis the wrong way to go?Linked issue
CO-327
Type of change
Developer checklist
Testing checklist
QA Steps
Screenshots (if available)