balazsbotond / urlcat

A URL builder library for JavaScript.
https://urlcat.org
MIT License
1.83k stars 57 forks source link

Donโ€™t allow missing, null or undefined path parameters,issue: 13 #15

Closed harshilparmar closed 4 years ago

harshilparmar commented 4 years ago

Summary

13 is solved!! Please let me know,if there any changes to done.Also trimmed value to check missing value.

balazsbotond commented 4 years ago

Thanks! I will have time to review this later today.

harshilparmar commented 4 years ago

@balazsbotond Done brother!! and One last question

it('Can\'t handle blank string as param', () => {
    expect(() => urlcat('http://example.com/path/:p', { p: " " }))
      .to.throw(TypeError, "Path parameter p cannot be of type string. Allowed types are: boolean, string, number.");
  });

this will show, above error while passing blank string as param,what do you think? is this ok ? or we have to the change specific like cannot be type blank string?

balazsbotond commented 4 years ago

@balazsbotond Done brother!! and One last question

it('Can\'t handle blank string as param', () => {
    expect(() => urlcat('http://example.com/path/:p', { p: " " }))
      .to.throw(TypeError, "Path parameter p cannot be of type string. Allowed types are: boolean, string, number.");
  });

this will show, above error while passing blank string as param,what do you think? is this ok ? or we have to the change specific like cannot be type blank string?

I think the error message in this case should be Path parameter ${key} cannot be an empty string.

harshilparmar commented 4 years ago

@balazsbotond Done brother!! and One last question

it('Can\'t handle blank string as param', () => {
    expect(() => urlcat('http://example.com/path/:p', { p: " " }))
      .to.throw(TypeError, "Path parameter p cannot be of type string. Allowed types are: boolean, string, number.");
  });

this will show, above error while passing blank string as param,what do you think? is this ok ? or we have to the change specific like cannot be type blank string?

I think the error message in this case should be Path parameter ${key} cannot be an empty string.

But for this We have to make separate error message !! Is this viable ??

balazsbotond commented 4 years ago

But for this We have to make separate error message !! Is this viable ??

Yes, this needs a separate if statement and error message.

harshilparmar commented 4 years ago

But for this We have to make separate error message !! Is this viable ??

Yes, this needs a separate if statement and error message.

@balazsbotond ohh, thanks for solving doubt.I will to that.

balazsbotond commented 4 years ago

Glad I could help :)

harshilparmar commented 4 years ago

@balazsbotond it's done!!

balazsbotond commented 4 years ago

Great, thank you! ๐Ÿ‘ I will have time to review it tomorrow morning.

balazsbotond commented 4 years ago

Everything looks good to me now. Thank you for your work on this issue. ๐ŸŽ‰

harshilparmar commented 4 years ago

Everything looks good to me now. Thank you for your work on this issue. ๐ŸŽ‰

it's my pleasure ๐Ÿ’ฏ