Skip to content

Fix query param generation#3

Merged
stoeffel merged 3 commits intomasterfrom
fix-query-param-generation
Oct 22, 2019
Merged

Fix query param generation#3
stoeffel merged 3 commits intomasterfrom
fix-query-param-generation

Conversation

@jwoudenberg
Copy link
Copy Markdown

@jwoudenberg jwoudenberg commented Oct 22, 2019

The generated ruby code for QueryFlags in Servant contains an error. This PR fixes it, and adds some golden tests too!

Creates space for other test suites in the `test` directory, such as the
golden test I'm about to add.
These test (and document) what code generation currently looks like for
various Servant API constructors.
Query flags are the Http equivalent of booleans. You can either set them
or you cannot. Currently the generated ruby code treats them as values:
You pass in a string and it gets added to the query string.

This ensures query flags in a servant API are modeled as booleans in the
generated ruby code.
@jwoudenberg jwoudenberg requested a review from stoeffel October 22, 2019 14:28
Copy link
Copy Markdown

@stoeffel stoeffel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code looks great, but we use the branch next-version in our codebase. maybe we should merge that into master and use master, since the upstream repo still has a few prs from us open.

@jwoudenberg
Copy link
Copy Markdown
Author

@stoeffel I merged next-version into our master a while ago, because the Nix based setup has trouble fetching commits from non-default branches.

Copy link
Copy Markdown

@stoeffel stoeffel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@stoeffel stoeffel merged commit 953e801 into master Oct 22, 2019
@stoeffel stoeffel deleted the fix-query-param-generation branch October 22, 2019 18:57
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