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

Encode URL parameters #117

Merged
merged 1 commit into from
Jun 13, 2017
Merged

Encode URL parameters #117

merged 1 commit into from
Jun 13, 2017

Conversation

xps
Copy link
Contributor

@xps xps commented Nov 1, 2016

This is a change so that URL parameters are properly encoded. Previously a call such as:

CreateItem("John&Johnson")

Would produce a request with the following URL:

.../create-item?name=John&Johnson

(i.e, the ampersand isn't encoded and the server only receives the first part of the string, "John", as the parameter value).

Strangely there was already a couple of EncodeParam() methods but they weren't called anywhere, so I just made sure they were applied. I am a bit puzzled as to how this issue could have stayed undetected so I might be missing something.

There was code to format dates with .ToString("o") so I simply updated the overload of EncodeParam() that deals with dates to use the same format.

The changes in the .cs file are auto-generated, the real changes are in the .tt file.

I am hoping that this can be merged, and that the NuGet packages can be updated soonish. Let me know if I can do anything else to assist.

@Korporal
Copy link

@xps - I see your pull request is rather old! Have you had challenges with thus repo accepting pull requests? The change in your PR, do you now just use your fork or did the same change somehow get made into the main repo by someone else?

@Korporal Korporal mentioned this pull request Jun 10, 2017
@xps
Copy link
Contributor Author

xps commented Jun 11, 2017

@Korporal - I haven't heard from the maintainers after submitting my pull request, and there hasn't been any commits to the main branch since then either. At this point, I do use my own fork.

@Korporal
Copy link

@xps - I did communicate with the owner a little and it seems that his level of involvement or enthusiasm is pretty much what it is. This is a shame because it discourages others from submitting changes and eventually makes the repo less attractive because there's no real effort being put into improving the code.

I posted this question too:

#121

and haven't seen any reply so I'm now wondering whether I should embark on my own effort, one that I'd make an effort to maintain...

@faniereynders
Copy link
Member

faniereynders commented Jun 13, 2017 via email

@Korporal
Copy link

@faniereynders - Thanks for responding, but you wrote:

"but it still stays a community effort."

and people are submitting pull requests and issues that you don't have time to respond to. Yes it's a community effort but the community does have some reliance on the owner(s) to approve/deny pull requests!

I don't see how you can expect the "community" to do much when they must eventually wait for the owner(s) to respond to pull requests?

If you're too busy (which you clearly are) then I don't see how we can contribute much to your project, all people can do is submit pull requests and wait - or make their fork more active than your repo and so eventually take over the effort.

@faniereynders
Copy link
Member

To get back to this PR, I'm going to merge it back to master. Many thanks @xps! @Korporal let's continue this discussion on the Gitter space https://gitter.im/RestCode

@faniereynders faniereynders merged commit 08f33eb into RestCode:master Jun 13, 2017
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.

3 participants