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

Added quotation marks to the serialized header values #144

Merged
merged 2 commits into from May 11, 2018

Conversation

3 participants
@Cellane
Copy link
Contributor

Cellane commented May 8, 2018

… at least I hope so 😅

This tiny PR is hopefully a fix to vapor/multipart#22 and to two days of banging my head against a wall.

@Cellane

This comment has been minimized.

Copy link
Contributor Author

Cellane commented May 8, 2018

It seems that if we decide to go for this, some tests will have to be updated. I’m not 100% sure if I’m reading the build log correctly but it’s possible that only this test is failing? If so, I can prepare another PR for that in the vapor/vapor repository.

@Cellane Cellane changed the title Added quotation marks to the serialized values Added quotation marks to the serialized header values May 8, 2018

@tanner0101
Copy link
Member

tanner0101 left a comment

Code LGTM. 🙌 I omitted the quotes originally to save a couple of bytes but looks like we need 'em. Gotta love the web haha. As you mentioned, we will also need to fix the test here: https://github.com/vapor/vapor/blob/master/Tests/VaporTests/ApplicationTests.swift#L199-L222

We be great if we could get a PR up for that before merging this so that Vapor's tests aren't broken.

@tanner0101

This comment has been minimized.

Copy link
Member

tanner0101 commented May 9, 2018

BTW, you can change which branch of Vapor is checked out during tests here: https://github.com/vapor/core/blob/master/circle.yml#L40

We should change this to the branch of Vapor that you apply the fix to, that way we can get green checks here before merging.

I've invited you to the contributors team so you should be able to create local branches now. 👍

@tanner0101 tanner0101 self-assigned this May 9, 2018

@Cellane

This comment has been minimized.

Copy link
Contributor Author

Cellane commented May 9, 2018

Aah, that’s how you do that! Okay, will submit PR for the tests by creating new branch directly on vapor/vapor instead of Cellane/vapor, then change circle.yml on this PR.

Can the changed circle.yml then be excluded from the final merge though? Apologies, this is the first time contributing in this cross-repository fashion.

@tanner0101

This comment has been minimized.

Copy link
Member

tanner0101 commented May 9, 2018

@Cellane once the merge goes through I just change it on master manually. Small price to pay for making sure everything is works haha.

Cellane added a commit to vapor/vapor that referenced this pull request May 9, 2018

@Cellane Cellane referenced this pull request May 9, 2018

Merged

Fixed tests to reflect changes in vapor/core#144 #1668

4 of 4 tasks complete

@tanner0101 tanner0101 added this to the 3.1.7 milestone May 11, 2018

@tanner0101

This comment has been minimized.

Copy link
Member

tanner0101 commented May 11, 2018

Thanks!

@tanner0101 tanner0101 merged commit d859f2b into vapor:master May 11, 2018

9 checks passed

ci/circleci: linux Your tests passed on CircleCI!
Details
ci/circleci: linux-fluent-mysql Your tests passed on CircleCI!
Details
ci/circleci: linux-fluent-postgresql Your tests passed on CircleCI!
Details
ci/circleci: linux-fluent-sqlite Your tests passed on CircleCI!
Details
ci/circleci: linux-jwt Your tests passed on CircleCI!
Details
ci/circleci: linux-leaf Your tests passed on CircleCI!
Details
ci/circleci: linux-redis Your tests passed on CircleCI!
Details
ci/circleci: linux-release Your tests passed on CircleCI!
Details
ci/circleci: linux-vapor Your tests passed on CircleCI!
Details
@penny-coin

This comment has been minimized.

Copy link

penny-coin commented May 11, 2018

Hey @Cellane, you just merged a pull request, have a coin!

You now have 68 coins.

@Cellane Cellane deleted the Cellane:quote-header-values branch May 11, 2018

tanner0101 added a commit to vapor/vapor that referenced this pull request May 12, 2018

@Cellane Cellane referenced this pull request May 14, 2018

Closed

Missing quotes from values #22

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment