Skip to content

Fix/fix 52 ‘#export()'depends on ‘{‘ location#65

Merged
loganwright merged 4 commits intovapor:masterfrom
plummer:fix/Fix-52-‘#export()'depends-on-‘{‘-location
May 16, 2017

Hidden character warning

The head ref may contain hidden characters: "fix/Fix-52-\u2018#export()'depends-on-\u2018{\u2018-location"
Merged

Fix/fix 52 ‘#export()'depends on ‘{‘ location#65
loganwright merged 4 commits intovapor:masterfrom
plummer:fix/Fix-52-‘#export()'depends-on-‘{‘-location

Conversation

@plummer
Copy link
Copy Markdown

@plummer plummer commented May 16, 2017

All tests passing.

Please let me know any feedback or if I've totally missed the mark on this one. Thanks.

#52

}

// Move through any redundant whitespace
while current?.isWhitespace == true && next?.isWhitespace == true {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I believe there is a function on this object called skipWhitespace() if it exists, might be good to use that if behavior is same same.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Hi Logan, Thanks, I didn't see the method for this. It seems to be used in the buffer class in ParameterParser but using some functions from StaticDataBuffer, and will need a pretty solid refactor since it's not available. There is quite a lot of function calls going on in there - while I was trying to understand it right just now you merged the PR, so I think the simple conditional is ok?

import XCTest
@testable import Leaf

class BodyWhitespaceTests: XCTestCase {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We'll need to add these to Tests/LinuxMain.swift so they run properly, look at the pattern in that file for other tests and you should be a-ok.

@loganwright
Copy link
Copy Markdown
Member

@plummer looks great to me, thanks so much for putting this together! A couple of small tweaks, mostly getting the tests running on Linux and we'll merge this.

Thanks 👍

@loganwright loganwright merged commit ed5a280 into vapor:master May 16, 2017
@plummer plummer deleted the fix/Fix-52-‘#export()'depends-on-‘{‘-location branch May 16, 2017 11:36
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