ErikWittern / openapi-snippet

Generates code snippets for given Swagger / Open API documents
MIT License
115 stars 66 forks source link

Revise parameter handling for snippets #86

Closed michaelgwelch closed 2 years ago

michaelgwelch commented 2 years ago

This is a large PR, but each individual commit is small(ish). The test cases are what make some changes look large. Each commit addresses one (or two) issues at a time (usually).

This commit does several related things.

ErikWittern commented 2 years ago

@michaelgwelch Hey, wow, it seems you put quite a lot of work into this PR, addressing multiple issues at once. Thank you very much!

You did open and then close again a few other PRs. So just to be on the same page: is this one ready-for-review from your point of view? Once it is, I will try to provide a review quickly, but might need a few days given other obligations. Just wanted to be upfront about this.

Thanks again, and let me know when I start reviewing.

michaelgwelch commented 2 years ago

You did open and then close again a few other PRs. So just to be on the same page: is this one ready-for-review from your point of view? Once it is, I will try to provide a review quickly, but might need a few days given other obligations. Just wanted to be upfront about this.

Hi @ErikWittern, yes I'm sorry for spamming your repo. When I submitted the first PR I quickly realized that this work was larger than I imagined. And I really wanted to submit a PR with smaller commits.

You can start to review this. I tried to stay faithful to your style and direction but realize I may have gone astray from how you would have liked. Let me know how I can help.

michaelgwelch commented 2 years ago

I think the most problematic issue is the test organization. I exposed an internal openapi-to-har function because there were so many combinations I would have had to have checked if I went thru the OpenAPISnippets interface. Perhaps those tests should go in their own module that test openapi-to-har directly. Then I don't need to export createHarParameterObject from that module.

The test descriptions may need more work as well?

ErikWittern commented 2 years ago

@michaelgwelch I finally had some time looking over your changes. I am extremely grateful, they look fantastic!

You point out some specific areas of interest related to testing. Here are my thoughts:

Once more, thank you so much for the contribution! I will create a new release based on these changes.

michaelgwelch commented 2 years ago

Very glad to hear you were able to work your way thru it all. On my project we've been running with these changes (from my fork) since I posted the PR back in May without issue. So I'm hopeful the tests have provided adequate coverage. Will be glad to switch over to the next released version and off of my fork.