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

Xcode 10 support #644

Merged
merged 7 commits into from
Jun 12, 2018
Merged

Xcode 10 support #644

merged 7 commits into from
Jun 12, 2018

Conversation

ikesyo
Copy link
Member

@ikesyo ikesyo commented Jun 6, 2018

Checklist

  • Updated CHANGELOG.md.

/// - inner: The inner property which validated values are committed to.
/// - other: The property that `validator` depends on.
/// - validator: The closure to invoke for any proposed value to `self`.
public convenience init<U, E>(
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 capture of the compiler error

2018-06-06 14 35 28

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 following overload will be used instead after this removal:

public convenience init<Other: PropertyProtocol>(
_ inner: MutableProperty<Value>,
with other: Other,
_ validator: @escaping (Value, Other.Value) -> Decision
) {

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this makes sense 👍

Copy link
Contributor

@sharplet sharplet left a comment

Choose a reason for hiding this comment

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

This looks good to me 👍

@@ -2791,7 +2791,7 @@ class SignalProducerSpec: QuickSpec {
let producer = SignalProducer<Int, NoError>.never
.on(disposed: { disposed = true })

var replayedProducer = ImplicitlyUnwrappedOptional(producer.replayLazily(upTo: 1))
var replayedProducer = Optional(producer.replayLazily(upTo: 1))
Copy link
Contributor

Choose a reason for hiding this comment

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

So we were using it as a regular optional anyway? Huh.

@mdiep
Copy link
Contributor

mdiep commented Jun 7, 2018

Do you want to move the headers phase before the compile sources phase as part of this?

@ikesyo
Copy link
Member Author

ikesyo commented Jun 9, 2018

Do you want to move the headers phase before the compile sources phase as part of this?

Addressed in eb2bae1!

Copy link
Member

@NachoSoto NachoSoto left a comment

Choose a reason for hiding this comment

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

🙏

/// - inner: The inner property which validated values are committed to.
/// - other: The property that `validator` depends on.
/// - validator: The closure to invoke for any proposed value to `self`.
public convenience init<U, E>(
Copy link
Member

@andersio andersio Jun 10, 2018

Choose a reason for hiding this comment

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

This changes the semantic. This overload was supposed to be the primary one, and the removed one here should have been implemented in terms of that.

Edit: Dropping down to PropertyProtocol leads to only successfully validated values from other being considered, while the intended semantic is to propagate any proposed value (regardless of its validation outcome) so that live validations are always reacting to any user interaction made.

Copy link
Member Author

@ikesyo ikesyo Jun 11, 2018

Choose a reason for hiding this comment

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

So what should be done here to avoid the compilation error instead of the removal? @andersio

Copy link
Member Author

@ikesyo ikesyo Jun 11, 2018

Choose a reason for hiding this comment

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

Feel free pushing your changes on this branch.

@andersio andersio dismissed their stale review June 11, 2018 16:40

Made some changes on this branch.

@andersio andersio merged commit 4a80683 into master Jun 12, 2018
@andersio andersio deleted the xcode-10 branch June 12, 2018 21:02
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

5 participants