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

Handle array parameters when compiling params #94

Merged
merged 1 commit into from
Aug 18, 2017

Conversation

kylef
Copy link
Member

@kylef kylef commented Aug 17, 2017

This PR also refactors the parameter compiler to accept Minim Elements directly and internally uses Minim elements to simplify the logic and to make it easier to fix the bug.

When the given input is not a minim element it will be converted, this conversion is temporary and should be removed once Dredd Transactions completely consumes Minim elements.

Fixes #90

This commit refactors the parameter compiler to accept Minim
Elements directly and internally uses Minim elements. When the given
input is not a minim element it will be converted.
parameters[name] =
required: 'required' in typeAttributes
default: value.attributes.get('default')?.toValue()
example: value.toValue() or values[0]
Copy link
Member Author

Choose a reason for hiding this comment

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

The problem lies around the example and default values. For an array these would've been an array of elements. The function returned example and default as an array of elements and thus expansion would expand these array of element objects instead of array of values.

@honzajavorek
Copy link
Contributor

Oh, I have compile-params.coffee ported to minim already in #85 😄 But this one contains the fix and also a lot of heroic effort of a Swift developer writing CoffeeScript, I can't imagine not merging this with higher priority.

Hopefully I'll be able to unite the changes somehow in my PR later on.

Copy link
Contributor

@honzajavorek honzajavorek left a comment

Choose a reason for hiding this comment

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

This is amazing, especially many thanks for the tests! 👍 🛠


parameters = compileParams(hrefVariables)

expect(parameters).to.deep.equal({
Copy link
Contributor

Choose a reason for hiding this comment

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

We use assert everywhere in Dredd, but that's simple change and can be done later.

@honzajavorek honzajavorek merged commit 4a0067c into master Aug 18, 2017
@honzajavorek honzajavorek deleted the kylef/compileParams branch August 18, 2017 08:32
@honzajavorek
Copy link
Contributor

This will result in a patch release, which should propagate to the latest Dredd automatically. I'd pin higher version of Dredd Transactions in Dredd only when #93 is fixed and the fix released as well.

honzajavorek added a commit that referenced this pull request Aug 22, 2017
Previously, for some legacy reasons, the code for compiling parameters stored
the array of possible values in a structure like this: [{'value': 123}, ...]
@kylef did not know that and did not notice this when rewriting the code
to use the minim library internally, so his changes worked with plain arrays
of values instead: [123, ...] This broke the subsequent code. Interestingly,
neither Dredd Transaction tests or Dredd tests caught this mistake. Especially
Dredd Transactions integration tests were not checking for extraneous warnings
and errors, so the problem slipped through unnoticed. This commit changes the
internal structure for storing the array of values to a plain array.

#94
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.

None yet

2 participants