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

Support passing HTTP method #7

Merged
merged 1 commit into from
Feb 17, 2021
Merged

Conversation

tomers
Copy link

@tomers tomers commented Feb 4, 2021

Hi, thanks for releasing a new version! I would be grateful if you could do the same now...

@fim
Copy link
Owner

fim commented Feb 8, 2021

Hey, it'd be nice if you could an idea of what's the end state you'd like to get to and batch all your changes in one PR along with a write up of what this new feature provides.

As is, it seems like these are small one off changes that do not benefit the project as a whole, at least not with a specific direction in mind and also require time and effort to merge/release.

If this is an ongoing effort, it'd make sense for you to use your own fork and implement all the changes needed there and then create a PR for everything at once rather than for each addition individually.

Hope this makes sense

@tomers
Copy link
Author

tomers commented Feb 15, 2021

Hi, any news on this? I'll be grateful if you could you review this. Thanks a lot!

@fim
Copy link
Owner

fim commented Feb 15, 2021

Please check my previous comment

@tomers
Copy link
Author

tomers commented Feb 15, 2021

Hi Serafeim,
There is no great plan here. I have no further changes that I plan to ask for.
I use the ICap client to test my icap, which happen to be more generic from what the current client expected, which is to POST a file to the ICap. In my case there is no file involved, and the HTTP method might not be a POST action. I was surprised to see the HTTP method hard-coded in the client's code, but I guessed nobody ever needed it up until this point.
I did not notice this when I posted my first merge. In any case I did not imagine it should pose any issue for the maintainer to release a minor version update, and also I am used to submit different changes in a separate PRs, which I did.

I hope you'll agree to merge this rather simple commit which makes the tool more flexible.

@fim fim merged commit ff015cd into fim:master Feb 17, 2021
@fim
Copy link
Owner

fim commented Feb 17, 2021

I clicked and merged this by mistake so I had to roll back master branch.

This won't even compile. I will get back with a more detailed review but feel free to fix and submit a new PR along with a usability test to make sure it's doing what's expected.

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