search

oliyh / martian

The HTTP abstraction library for Clojure/script, supporting OpenAPI, Swagger, Schema, re-frame and more
MIT License
529 stars 44 forks source link

Add OpenAPI YAML definition support. #132

Closed bombaywalla closed 2 years ago

bombaywalla commented 2 years ago

For CLJ only.

Addresses #125

bombaywalla commented 2 years ago

This PR is ready for review.

bombaywalla commented 2 years ago

Hi @oliyh

I got an email from you (via github) with this comment: "Cheshire is still needed here". But I don't see the corresponding comment here in the PR. So, am not sure which file(s) you are referring to.

It seemed to me that the project.clj for martian.core is including cheshire so I removed it from the project.cljs of modules that had it mentioned and that also included martian.core. This mirrors the change you asked for in moving the clj-yaml dep from the modules to martian.core.

Since all the tests passed, I thought that what I did was okay.

Happy to revert the cheshire deps in all the project.cljs if that's what you want. Please let me know. Thanks

oliyh commented 2 years ago

Hi,

I was looking at the http-kit module which has a require for Cheshire, so it should still have Cheshire in the project clj rather than relying on the user also having a dependency on core (even though they realistically would)

bombaywalla commented 2 years ago

Sorry if I'm being dense, but...

The project.clj for martian.http-kit has a dep for martian.core, which has a dep for cheshire. So, if a user used martian.http-kit (but did not explicitly include martian.core) they'd still get cheshire, right?

oliyh commented 2 years ago

Hi @bombaywalla Yes, it would still get cheshire on the classpath but transitively. If martian.http-kit only made calls to martian.core, and martian.core made all the calls to cheshire I'd be fine with that, but martian.http-kit explicitly requires cheshire and makes calls to it. To mirror this there should be an explicit dependency on cheshire in martian.http-kit.

This will avoid the scenario where some developer removes all usage of cheshire in martian.core, and removes it from the core project.clj thinking it's no longer used. This would unintentionally and unknowingly break compilation of martian.http-kit.

Many thanks

oliyh commented 2 years ago

Same for clj-http-lite as well. Hato and clj-http don't need it as those libraries natively convert json to clojure.

Thanks

bombaywalla commented 2 years ago

Okay. Makes sense. Added cheshire back to httpkit and clj-http. Please review.

oliyh commented 2 years ago

Hi,

Sorry I made a half mistake in my previous comment - clj-http-lite does need cheshire, clj-http doesn't.

Once that's done I think this is good to go, thank you.

bombaywalla commented 2 years ago

Fixed.

oliyh commented 2 years ago

Thank you!

oliyh commented 2 years ago

I'll build a snapshot to clojars tomorrow

oliyh commented 2 years ago

Hi, this is now in 0.1.21-SNAPSHOT

bombaywalla commented 2 years ago

Thanks.