Servant docs curl #1401
Servant docs curl #1401
Conversation
|
I'm sorry but I don't understand how this build failed: https://travis-ci.org/github/haskell-servant/servant/jobs/762651210 Everything says it exited with exit code 0, but the overall process failed somehow? |
|
Okay it looks like it failed compiling
But if I'm not mistaken this is from a different package and it's only compiled as part of |
|
Hi Dan, Thanks for your contribution. The CI is actually in an unstable state. We need to fix it before you being able to pass all checks on it (we are moving out from Travis). (see this) This one will also need to be merged because of the dependency loop between servant-quickcheck and servant. I will keep you in touch. |
…nto servant-docs-curl
|
@akhesaCaro just wondering, any feedback I should address? |
|
Hi @akhesaCaro sorry to keep pinging you, but I'm just wondering if you've seen this? |
|
I am sorry @dfithian , I haven't had the time to study your PR yet. |
|
Awesome, very excited for this! |
gdeest
force-pushed
the
servant-docs-curl
branch
from
9346d42 to
d0a2cc2
Compare
gdeest
force-pushed
the
servant-docs-curl
branch
from
d0a2cc2 to
c00fe3b
Compare
|
Hi! The activity on this PR is a little dead. Any things I should do to make it acceptable to merge? |
|
@gdeest @akhesaCaro what needs to be done to get this PR in? |
|
Lovely idea. This seems to be a breaking change, so will require a major bump, but it does look fine to me. Perhaps @akhesaCaro will have some feedback when she gets a chance to look into this. |
|
I don't really understand why there hasn't even been a review. If you don't want a change, or you have feedback, all you have to do is comment. Saying "this change isn't welcome" is fine with me, if that's actually the feeling. If there's something to be done to actually make it acceptable, it would be great to get that feedback. |
|
@akhesaCaro would love to get your thoughts. |
|
@dfithian The reason you haven't got a review is the simplest possible, we've all been quite busy I'm afraid. I know this isn't ideal from a contributor's perspective, but for the past 6 years now, the biggest challenge has been to have enough people around to deal with the various (and welcome!) bug reports and contributions at a reasonable pace. In fact I'm one of the original authors but had to take a break from the project at some point because I couldn't handle the activity anymore, in addition to my personal & professional life, after having spent hundreds of evenings over the years on dealing with various servant related things (bugs, reviews, patches, helping out users). I only recently started coming back to the project. I don't mean to complain by saying this, most definitely not. I'm just trying to find a way to say that your contributions are most definitely welcome, and that the delay in giving them the right amount of focus and care is not due to the patch or you, but simply (and perhaps disappointingly, for you) the bandwidth of the people who are currently volunteering their time to help keep things running around here. More help would definitely be welcome, of course. I'll nonetheless try to find a bit of time (tonight?) to do a careful review of your patch, depending on how much time my kids give me. :-) |
| mkHeaderStr (hdrName, hdrVal) = | ||
| " -H '" ++ cs (CI.original hdrName) ++ ": " ++ | ||
| cs hdrVal ++ "' \\" | ||
| mkReqBodyStr (_, _, body) = " -d " ++ cs body ++ " \\" |
There was a problem hiding this comment.
Will this correctly escape the request body in case it contains "s?
There was a problem hiding this comment.
That's a great point. Same with the headers. However I think it's up to the implementor to escape those quotes in the ToSample instance. Do you agree?
There was a problem hiding this comment.
Hmm, well proper escaping will take place for whatever content types are relevant for the request/response bodies (e.g "s are escaped in sample JSON bodies), so I would be inclined to say that the escaping would be fine. But since you're more familiar with this patch than myself for obvious reasons, I was essentially asking for your confirmation. It simply occurred to me that your patch was introducing bash escaping to the mix.
And indeed the same question stands for headers.
(I don't think relying on each sample doing things right is the best way out of this question though.)
There was a problem hiding this comment.
I don't see why the body would be different in this case than in the case where an actual request was being sent, meaning that I expect that quotes would be escaped correctly.
I tested this out, and in fact it doesn't quite do the correct thing; with a (pretty) JSON body, it did something like this:
-d {
"foo": "bar"
} \So I think we should actually wrap it in single quotes - then we won't have to worry about escaping " anyway. I think I was fooled by the Greet example because it was a JSON string.
Edit: And we don't have to worry about the headers - I checked, and they already are wrapped in single quotes.
There was a problem hiding this comment.
I think this should be good now
There was a problem hiding this comment.
If we wrap everything up in single quote, are we sure things will go smoothly in the presence of single quotes inside e.g the request body?
There was a problem hiding this comment.
I don't think so. If there are single quotes inside a JSON request body, they'll show up in the snippet unescaped.
There was a problem hiding this comment.
Hmm, is there something we can do to guarantee that people will be able to copy/paste any curl commands produced by this code and have it work? This is really the last blocker for me, for this PR, which is otherwise ready to land. Sorry to be a bit of a PITA here, but I'm just thinking about the scenario where in a professional setting people start generating docs with this but other folks in the company would complain about commands not working, etc.
There was a problem hiding this comment.
I guess I could use double quotes and escape double quotes in the block. I'm always wary of doing these things by hand but I'll give it a try
There was a problem hiding this comment.
Okay, done, and greet.md updated.
…servant-docs-curl
|
I totally understand that maintainers are busy! I was just hoping for some update saying "yes, we think this is a good/bad change" - that way I could either close it if it was bad or know that someone was thinking about it if it was good. However, a review is even better! Thanks so much! |
|
@dfithian I did voice my enthusiasm back some time ago =) I really meant it. Auto-generated Anyway I'm happy that everything's fine now and that your patch is on its way to |
Optionally generate sample cURL requests from the Servant description. There's a breaking change which requires manual changes of
headersto supply a sample value.I have updated
greet.hsandgreet.mdwith the changes.