Closed vermaxik closed 4 years ago
Hey @vermaxik ,
First thanks for opening this issue!
If you know what is the problem and want to send a PR that would be great. Otherwise I will try to fix this the next day or so as this will definitely impact other users.
@vermaxik Thanks for the debugging, that helps a lot. It seems to be an issue caused by keyword lists being passed through directly to the encoder rather than first being converted to a map-like structure.
@edgurgel I'd be happy to help investigate further!
Yeah I think the issue is that we were using keyword/lists as you said!
We might need to transform them into maps first if they are Keywords
A fairly common pattern in the codebase seems to be accepting either a keyword list or a map as a body; this means that many requests will now be encoded incorrectly when the former is passed in.
I think it may be a good idea to convert keyword lists to maps in each function, thus keeping the current API intact? I also think using maps for the data intended as the body is better than keyword lists as it more accurately represents the JSON (no duplicate keys for instance).
Agreed, @niklaslong!
We might be fine by simply calling Map.new(kw_list)
if the argument is a keyword list?
I wonder if we might need to use Keyword.keyword?(list)
instead of just checking if it's a list ? I'm not sure if there's some GitHub API that accepts a JSON body as a single list/array?
body: ["a", "b", "c"]
We might want to use this to ensure that the request body is being matched? https://github.com/parroty/exvcr#matching-against-request-body
@edgurgel yes, we should definitely be testing against the request body!
I think we should check if it is a keyword list using Keyword.keyword?(list)
just to be safe. After all, encoding a list would be entirely valid.
I think #194 is perhaps a good first step but some longer term changes will have to be implemented.
I think we should:
body
in all testsbody
of the request. If I'm not mistaken, the API isn't consistent in this regard and homogenising it may be a nice refactor? @niklaslong yeah I think it's the right time to break the API and accept only maps. 2.0.0 will be retired anyway as it's a broken release. We can release 2.0.1 having it with just maps?
@niklaslong I will see how far I can get changing this and I will open a PR.
@edgurgel sounds good. I'd be happy to help if needed, just let me know!
2.0.1 is out! Hopefully fixed all the issues. I've manually tested a few commands and they all seem to work fine and if a keyword is used it complains that a map is expected. Thanks, everyone!
Hi folks,
I've updated project to 2.0.0 version and got issue with creating new repo:
Small debugging showed that
post
request generate request not correctly, it sends (look on body):Expected behavior:
@niklaslong @edgurgel would you like to fix it, or I can help with a fix?
Thank you!