Skip to content

Feature: Add Custom HTTP Headers and Method (via CLI and Programmatically) #713

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

Closed

Conversation

ericzorn93
Copy link
Contributor

@ericzorn93 ericzorn93 commented Aug 6, 2021

This feature allows the user to send custom HTTP headers to the JS API in order to introspect and parse the remote URL Swagger/OpenAPI schema. This has been updated for the CLI format as well. These changes will not cause any breaking changes to the API and/or user.

All tests are passing.

@ericzorn93 ericzorn93 marked this pull request as ready for review August 6, 2021 04:11
@ericzorn93
Copy link
Contributor Author

Hey @drwpow, it won't let me add reviewers to this PR for some reason.

@ericzorn93 ericzorn93 changed the title Feature: Add Custom HTTP Headers Feature: Add Custom HTTP Headers and Method Aug 9, 2021
@drwpow
Copy link
Contributor

drwpow commented Aug 12, 2021

Hey @drwpow, it won't let me add reviewers to this PR for some reason.

I believe you have to have repo access to do that. But feel free to just @- mention anyone, or I can add them myself.

@drwpow
Copy link
Contributor

drwpow commented Aug 12, 2021

@ericzorn93 this is great! Very well-thought out, great execution, and thanks for improving the tests, too.

I’m happy to approve & merge this but let me know if you wanted additional reviewers as well.

@ericzorn93
Copy link
Contributor Author

ericzorn93 commented Aug 12, 2021

@drwpow you can feel free to merge, I may have other PRs in the future refining some tests. However, this functionality would be a great help. Thank you 😀

Also, cc: @gr2m @garyposter @scvnathan as additional approvers

@gr2m
Copy link
Contributor

gr2m commented Aug 12, 2021

I'm sorry I don't have the bandwidth to review. But we gotta trust the tests. If something breaks, that's the fault of the test setup, not your PR :) Looks like great work, thank you!

@ericzorn93
Copy link
Contributor Author

ericzorn93 commented Aug 12, 2021

@gr2m Thank you. If we are good, do you or another maintainer mind merging as soon as you have a chance?

@ericzorn93
Copy link
Contributor Author

@drwpow Sorry to bother you again, but if this PR is safe to merge, do you mind merging to master? I think this would be a great help for my personal use case and others as well 👍

@ericzorn93 ericzorn93 requested a review from drwpow August 20, 2021 17:54
Copy link
Contributor

@mbelsky mbelsky left a comment

Choose a reason for hiding this comment

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

This is a great feature that I'd really would like to have in this tool.

@gr2m
Copy link
Contributor

gr2m commented Aug 29, 2021

Once you two are in agreement and confirmed that it's working, can you please ping Drew? It's up to him whether the added complexity is something he wants to maintain in the long term. You have my blessing, but I'm just helping out :)

@codecov
Copy link

codecov bot commented Aug 29, 2021

Codecov Report

Merging #713 (eaf981c) into main (c35278a) will decrease coverage by 0.28%.
The diff coverage is 89.13%.

❗ Current head eaf981c differs from pull request most recent head 1b13ccf. Consider uploading reports for the commit 1b13ccf to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main     #713      +/-   ##
==========================================
- Coverage   96.35%   96.06%   -0.29%     
==========================================
  Files          12       13       +1     
  Lines         548      585      +37     
  Branches      194      201       +7     
==========================================
+ Hits          528      562      +34     
- Misses         19       22       +3     
  Partials        1        1              
Impacted Files Coverage Δ
src/load.ts 91.24% <86.11%> (-0.35%) ⬇️
prettier.config.js 100.00% <100.00%> (ø)
src/index.ts 96.66% <100.00%> (ø)
src/utils.ts 96.87% <100.00%> (+0.32%) ⬆️
src/transform/schema.ts 98.98% <0.00%> (ø)

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 56cc872...1b13ccf. Read the comment docs.

@gr2m
Copy link
Contributor

gr2m commented Aug 29, 2021

sounds like you'll have to bump the coverage ☝🏼 it shouldn't drop

@ericzorn93
Copy link
Contributor Author

@gr2m I have added a number of small tests for utility functions to attempt to bump coverage. I don't know how much more I can really increase unit test coverage of the load function without adding insignificant granular unit tests

@ericzorn93 ericzorn93 changed the title Feature: Add Custom HTTP Headers and Method Feature: Add Custom HTTP Headers and Method (via CLI and Programmatically) Aug 29, 2021
@ericzorn93
Copy link
Contributor Author

@gr2m all test coverage has been updated

@ericzorn93
Copy link
Contributor Author

ericzorn93 commented Sep 5, 2021

  • Hey @gr2m, @mbelsky, and @drwpow. Sorry to be pushy, but my PR should be good to go with all requested changes and increased test coverage. Now, you can add headers individually like the CURL CLI, or via a JSON string.

Do you mind merging? Thank you so much.

This functionality would be great in my current project 🎉

@gr2m
Copy link
Contributor

gr2m commented Sep 5, 2021

This functionality would be great in my current project 🎉

To take the pressure of, maybe publish your own fork for the time being? I don't know when @drwpow will have the time to review, and it's bad etiquette in independent Open Source projects to pressure maintainers like that, please don't do that.

@ericzorn93
Copy link
Contributor Author

@gr2m I completely understand and did not think of that tbh. I will do this in the meantime. I apologize for any pressure I have put on the maintainers

@drwpow drwpow force-pushed the feature-add-custom-http-headers branch from eaf981c to 1b13ccf Compare September 30, 2021 17:56
@drwpow drwpow closed this Sep 30, 2021
@drwpow
Copy link
Contributor

drwpow commented Sep 30, 2021

@ericzorn93 sorry for the delay on this! This library has been difficult for me to maintain the past few months. I went ahead and approved and merged this in #764, with a few changes as-noted. See that PR for more details.

This approach of merging your work in another PR is a bit unorthodox, and I apologize. Please check that off as a failure on my part to give you a timely review! If you’d like to adjust anything from my revisions, please open up another PR.

Overall I wanted to say again what a quality PR this was! Your JS Docs are excellent, your comments are excellent, and I am amazed at the amount of thought & care you put into this PR. And I apologize again for not merging this in sooner because it is quality work.

I’ll be shipping a new minor version with this feature soon!

@drwpow
Copy link
Contributor

drwpow commented Sep 30, 2021

@all-contributors please add @ericzorn93 for code, test, docs

@allcontributors
Copy link
Contributor

@drwpow

I've put up a pull request to add @ericzorn93! 🎉

@ericzorn93
Copy link
Contributor Author

@drwpow No worries at all. I completely understand. I apologize for coming across pushy, I was just trying to incorporate this on a script that I have been working on.

I completely understand that maintaining widely used open source software can be a difficult task and very time consuming. This is a great repo!

Thanks for the kind words and I look forward to contributing more in the future 😀.

Thanks,
Eric

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants