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
Add headers1A property to the AST for both API Blueprint and Refract adapters #83
Conversation
Looks good, but please add minor test for this. |
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.
Please add a test for checking that when you're putting two headers, those get persisted in the new property.
CHANGELOG.md
Outdated
|
||
### Enhancements | ||
|
||
- `headers1A` property added to the Request / Response transaction pairs for both API Blueprint and Refract adapters. |
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.
Please explain why we made that (not losing the headers)
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.
Addressed.
requestBody = trimLastNewline(if _.content(httpRequestBody) then _.content(httpRequestBody) else '') | ||
requestSchema = trimLastNewline(if _.content(httpRequestBodySchemas) then _.content(httpRequestBodySchemas) else '') | ||
requestAuthSchemes = transformAuth(httpTransaction, options) | ||
|
||
[requestHeaders, requestHeaders1A] = getHeaders(httpRequest) |
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 we should return an object rather than an array - it's confusing.
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.
Actually, getHeaders
should probably only return headers
and we can add getHeaders1A
to return headers1A
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.
You decide on that, let's just not use arrays as a returning pattern as long it's not really same type.
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 agree that returning arrays with two (or more) different data structures is not very elegant and good for readibility, but I wanted to keep it DRY. I will add getHeaders1A
.
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.
Addressed.
test/transformation-test.coffee
Outdated
setCookieHeaders = ast.sections[0].resources[0].responses[0].headers1A.filter((item) -> | ||
item.name is 'Set-Cookie' | ||
) | ||
assert.equal(setCookieHeaders.length, 3) |
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.
Could you please check that the value is different for all these? (Make sure it's not being overwritten)
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.
Addressed.
@@ -1,6 +1,6 @@ | |||
{ | |||
"name": "@apiaryio/metamorphoses", | |||
"version": "0.13.7", | |||
"version": "0.13.8", |
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.
This is a minor release, not a patch. It's adding a new property.
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.
From semver
Major version zero (0.y.z) is for initial development. Anything may change at any time. The public API should not be considered stable.
When less than v1, generally, people follow the rule of doing minor changes as patch versions and major changes as minor versions.
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.
Ah - ok.
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.
- Bump to 0.14
- Add a test making sure the value is different.
I think we can move ahead! |
Changes made in this PR are needed for support of multiple headers (with the same key / header name) in the core app.