Skip to content
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

fix(types): Allow no relative url to fetch RESTful api list #342

Closed
wants to merge 5 commits into from

Conversation

krystofwoldrich
Copy link
Contributor

@krystofwoldrich krystofwoldrich commented Aug 25, 2022

fix(get): Allow no relative url to fetch RESTful api list

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change? (check one)

  • Yes
  • No

If yes, please describe the impact and migration path for existing applications:

The PR fulfills these requirements:

  • When resolving a specific issue, it's referenced in the PR's title (e.g. fix #xxx[,#xxx], where "xxx" is the issue number)
  • All tests are passing
  • New/updated tests are included

If adding a new feature, the PR's description includes:

  • A convincing reason for adding this feature (to avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it)

Other information:

I've noticed examples in the README.md, where the get method was used without any parameters. But the TS typing didn't allowed such a calls. In my opinion the call without any arguments looks cleaner than call with empty string to get list from a resource.

fix(get): Allow no relative url to fetch RESTful api list
@posva posva changed the title fix(get): Allow no relative url to fetch RESTful api list fix(types): Allow no relative url to fetch RESTful api list Aug 25, 2022
@codecov
Copy link

codecov bot commented Aug 25, 2022

Codecov Report

Merging #342 (faa6ff4) into main (69c9547) will not change coverage.
The diff coverage is 100.00%.

❗ Current head faa6ff4 differs from pull request most recent head fd88a34. Consider uploading reports for the commit fd88a34 to get more accurate results

@@           Coverage Diff           @@
##             main     #342   +/-   ##
=======================================
  Coverage   94.28%   94.28%           
=======================================
  Files           1        1           
  Lines          70       70           
  Branches       25       25           
=======================================
  Hits           66       66           
  Misses          4        4           
Impacted Files Coverage Δ
src/index.ts 94.28% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Owner

@posva posva left a comment

Choose a reason for hiding this comment

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

Thanks!

@krystofwoldrich
Copy link
Contributor Author

Okay. I've changed the typing and added a bit of logic to correctly recognise url and options. Also I've added 2 more tests.

Let me know, what do you think.

@krystofwoldrich
Copy link
Contributor Author

Any updates?

@krystofwoldrich
Copy link
Contributor Author

@posva Thanks for the tips. I've change it. I hope it's okay, you can take a look.

@posva
Copy link
Owner

posva commented Feb 27, 2023

Thank you, I added a few last changes and merged it in #379

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.

2 participants