-
-
Notifications
You must be signed in to change notification settings - Fork 414
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
Servant docs curl #1401
Merged
alpmestan
merged 16 commits into
haskell-servant:master
from
dfithian:servant-docs-curl
Aug 19, 2021
+110
−25
Merged
Servant docs curl #1401
Changes from 7 commits
Commits
Show all changes
16 commits
Select commit
Hold shift + click to select a range
50db93f
Generate sample curl request
a10af4e
Also generate sample request headers
72f2664
Undo changes to versioning based on contributing guidelines
92ea93e
Add changes to changelog
3453d64
Deal with reordering of fields
9346d42
Merge branch 'master' of https://github.com/haskell-servant/servant i…
97e8869
Generate sample curl request
1b99b70
Also generate sample request headers
d6f8906
Undo changes to versioning based on contributing guidelines
d8ce87b
Add changes to changelog
e6be76e
Deal with reordering of fields
fde701a
fixing DocSpec tests to make them compile
akhesaCaro c00fe3b
Refactor curlStr
bc15706
Merge branch 'servant-docs-curl' of github.com:dfithian/servant into …
967e745
Wrap the body in single quotes
19b4cca
Escape double quotes
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,6 +30,7 @@ doc/_build | |
doc/venv | ||
doc/tutorial/static/api.js | ||
doc/tutorial/static/jq.js | ||
shell.nix | ||
|
||
# nix | ||
result* | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
synopsis: Add sample cURL requests to generated documentation | ||
prs: #1401 | ||
|
||
description: { | ||
|
||
Add sample cURL requests to generated documentation. | ||
|
||
Those supplying changes to the Request `header` field manually using | ||
lenses will need to add a sample bytestring value. | ||
|
||
`headers <>~ ["unicorn"]` | ||
|
||
becomes | ||
|
||
`headers <>~ [("unicorn", "sample value")]` | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this correctly escape the request body in case it contains
"
s?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
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 theGreet
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be good now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, done, and greet.md updated.