Optional wrapper

[scvnathan]

I would rather that the default behavior be no wrapper, but that would be a breaking change.
Of course, I'm open to any other way to make wrapper optional

[codecov]

Codecov Report

Merging #33 into master will increase coverage by 0.08%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #33      +/-   ##
==========================================
+ Coverage   97.84%   97.93%   +0.08%     
==========================================
  Files           2        2              
  Lines          93       97       +4     
  Branches       34       37       +3     
==========================================
+ Hits           91       95       +4     
  Misses          2        2

Impacted Files	Coverage Δ
src/swagger-2.ts	97.8% <100%> (+0.1%)	⬆️

---

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 0337910...4df64b1. Read the comment docs.

[scvnathan]

This doesn't work with the CLI yet, working on a fix for that

[drwpow]

Rather than having this be a union type, maybe we should just have const shouldUseWrapper = options.wrapper !== undefined ? And we can just make this property optional.

[drwpow]

Ah sorry—brainfart. It is nice having declare namespace OpenAPI be the default. I’m fine with this as-is.

[drwpow]

This doesn't work with the CLI yet, working on a fix for that

Just changing the following in bin/cli.js should work?

wrapper: {
-  type: 'string',
+  type: 'string | boolean', 

Not sure how to specify true as a type, but unless I’m mistaken, passing true in your code should just return the default (declare namespace OpenAPI)

[drwpow]

Overall, this looks great! Happy to this once it’s working with the CLI

[scvnathan]

Hey @DangoDev I'll have some time today to work on this again, so I should have something later. The problem I was running into with the cli is that the value was coming in as a string so the triple equals check on options.wrapper wasn't working

[scvnathan]

Just changing the following in bin/cli.js should work?

wrapper: {
-  type: 'string',
+  type: 'string | boolean', 

Unfortunately this didn't work. I'm not sure if meow supports union types like that.

[drwpow]

I'm not sure if meow supports union types like that.

Oh you’re right. I haven’t used that before, but misread the meow docs. Hm. The CLI is just a wrapper around the function, so the CLI arguments don’t have to match exactly. Maybe you could pass a --nowrapper option or something?

[drwpow]

Also I think this is good enough to merge as-is; LMK if you’re interested in writing docs for this, or if not I can merge this and wrap up in a separate PR

[scvnathan]

Apologies for the lack of docs 😅, I can add some but I'm away from my computer at the moment.
Also if you prefer nowrapper I don't mind changing it.

Feel free to merge if you like as I probably won't be able to get to this until tonight.

[drwpow]

@scvnathan I’m in no rush! Just offering to help out if you’d like it. I’m happy to let you push this PR as far as you’d like / have time for, and I can do the rest. Just tag me whenever you’re done!

[drwpow]

This looks good to me! I’ll approve this, and let you merge when you’re happy with it. I’ll test it locally myself then publish a new version. Awesome work!

[MrCrunchwrap]

The cli --nowrapper option doesn't seem to work properly. I'm using it in my command and my types are still wrapped with declare namespace OpenAPI2

[drwpow]

@MrCrunchwrap Sorry—I’ve been traveling and latest npm release isn’t in-step with master, so that option isn’t available yet. I’m going to test it a bit more and then cut a new release soon.

[drwpow]

@MrCrunchwrap Just published version 1.3.0 that contains the --nowrapper option for the CLI, but if using npx it should automatically pick up on the new version. Beyond that, please let me know if you have any difficulty using this!