Skip to content

Conversation

@tanner0101
Copy link
Contributor

Make all members of HTTP{Request|Response}Head mutable.

Motivation:

Immutable members of structs (properties defined with let) can never be changed
even if the struct itself is mutable (defined withh var). This makes it difficult
to build rich convenience APIs on top of these structs.

Modifications:

Changed all 'let' stored properties on HTTP{Request|Response}Head to 'var'.

Result:

HTTP{Request|Response}Head are easier to use now.

Motivation:

Immutable members of structs (properties defined with let) can never be changed
even if the struct itself is mutable (defined withh var). This makes it difficult
to build rich convenience APIs on top of these structs.

Modifications:

Changed all 'let' stored properties on HTTP{Request|Response}Head to 'var'.

Result:

HTTP{Request|Response}Head are easier to use now.
@swift-nio-bot
Copy link

Can one of the admins verify this patch?

1 similar comment
@swift-nio-bot
Copy link

Can one of the admins verify this patch?

Copy link
Contributor

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

This seems reasonable to me.

@Lukasa
Copy link
Contributor

Lukasa commented Apr 17, 2018

@swift-nio-bot add to whitelist

@Lukasa Lukasa added the 🆕 semver/minor Adds new public API. label Apr 17, 2018
@Lukasa Lukasa added this to the 1.5.0 milestone Apr 17, 2018

// Internal representation of the URI.
private let rawURI: URI
private var rawURI: URI
Copy link
Member

Choose a reason for hiding this comment

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

This one is private so I think this makes no sense or I am missing something ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, that one could stay let.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I missed adding a set case to uri, that is added now (and relies on rawURI being mutable)

Copy link
Member

@weissi weissi left a comment

Choose a reason for hiding this comment

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

👍 thanks @tanner0101

/// The URI used on this request.
public var uri: String {
return String(uri: rawURI)
get { return String(uri: rawURI) }
Copy link
Member

Choose a reason for hiding this comment

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

we don't usually skip the newlines here but I don't mind. CC @normanmaurer / @Lukasa

Copy link
Member

Choose a reason for hiding this comment

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

I would prefer to add them just to make things consistent .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Consistency is king

@weissi
Copy link
Member

weissi commented Apr 17, 2018

@swift-nio-bot test this please

@normanmaurer normanmaurer merged commit 1360fc6 into apple:master Apr 18, 2018
@normanmaurer
Copy link
Member

@tanner0101 thanks!

@tanner0101 tanner0101 deleted the mutable-http-heads branch April 18, 2018 19:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🆕 semver/minor Adds new public API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants