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

@propertyWrapper Published #52

Merged
merged 20 commits into from
Sep 7, 2019
Merged

@propertyWrapper Published #52

merged 20 commits into from
Sep 7, 2019

Conversation

beastgrim
Copy link
Contributor

@beastgrim beastgrim commented Sep 1, 2019

Hello! I've implemented this interface. That allows use construction like this:
@Published var value: String?
And bind and subscribe:
$value.assign(to: \.text, on: textLabel)
I hope that will be helpful.

@OpenCombineBot
Copy link

OpenCombineBot commented Sep 1, 2019

LGTM

Generated by 🚫 Danger Swift against a2903f0

Copy link
Member

@broadwaylamb broadwaylamb left a comment

Choose a reason for hiding this comment

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

Thank you!

I've added some comments, but in order for it to be merged we'll need tests, also can you please address the bot's comments? :)

Sources/OpenCombine/Published.swift Show resolved Hide resolved
Sources/OpenCombine/Published.swift Outdated Show resolved Hide resolved
Sources/OpenCombine/Published.swift Outdated Show resolved Hide resolved
Sources/OpenCombine/Published.swift Outdated Show resolved Hide resolved
@broadwaylamb broadwaylamb added the missing functionality This functionality is present in Apple's Combine but we don't have it yet label Sep 1, 2019
@beastgrim
Copy link
Contributor Author

Sorry for inconvenience. It is first time when I do pull request to GitHub. I fixed all comments and also warnings from SwiftLint. It remains only to write tests.
When it is done what I should do next? Should I close this pull request and open another or there is another variants? Thanks!

@broadwaylamb
Copy link
Member

It is first time when I do pull request to GitHub

I’m flattered!

Should I close this pull request and open another or there is another variants?

When you write the tests, commit them to this branch, it is better if they come along with the feature that they test. No need to create a separate pull request for that :)

Copy link
Member

@broadwaylamb broadwaylamb left a comment

Choose a reason for hiding this comment

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

Can you please remove the interface of Published from the RemainingCombineInterface.swift so that we can track what's implemented and what's not?

I also see that your implementation lacks this subscript. Implementing it is non-trivial, so I suggest to add a stub marked as unavailable:

@available(*, unavailable, message: "This subscript is unavailable in OpenCombine yet")
public static subscript<EnclosingSelf: AnyObject>(
    _enclosingInstance object: EnclosingSelf,
    wrapped wrappedKeyPath: ReferenceWritableKeyPath<EnclosingSelf, Value>,
    storage storageKeyPath: ReferenceWritableKeyPath<EnclosingSelf, Published<Value>>
) -> Value {
    get { fatalError() }
    set { fatalError() }
}

Sources/OpenCombine/Published.swift Outdated Show resolved Hide resolved
Sources/OpenCombine/Published.swift Show resolved Hide resolved
Sources/OpenCombine/Published.swift Outdated Show resolved Hide resolved
Copy link
Contributor Author

@beastgrim beastgrim left a comment

Choose a reason for hiding this comment

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

I also see that your implementation lacks this subscript. Implementing it is non-trivial, so I suggest to add a stub marked as unavailable:

@available(*, unavailable, message: "This subscript is unavailable in OpenCombine yet")
public static subscript<EnclosingSelf: AnyObject>(
    _enclosingInstance object: EnclosingSelf,
    wrapped wrappedKeyPath: ReferenceWritableKeyPath<EnclosingSelf, Value>,
    storage storageKeyPath: ReferenceWritableKeyPath<EnclosingSelf, Published<Value>>
) -> Value {
    get { fatalError() }
    set { fatalError() }
}

I've added this subscript, but I didn't find that implementation in Apple Combine framework (Xcode 11.0 beta 6). I will be grateful If you give me more information about that functionality and how I can use that. Thanks!

Sources/OpenCombine/Published.swift Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Sep 3, 2019

Codecov Report

Merging #52 into master will increase coverage by 0.18%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #52      +/-   ##
==========================================
+ Coverage   97.55%   97.74%   +0.18%     
==========================================
  Files          42       43       +1     
  Lines        1679     1727      +48     
==========================================
+ Hits         1638     1688      +50     
+ Misses         41       39       -2
Impacted Files Coverage Δ
...enCombine/Publishers/Publishers.ReplaceError.swift 100% <0%> (ø)
...rces/OpenCombine/Publishers/Publishers.Print.swift 98.36% <0%> (+3.27%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9b9915b...a2903f0. Read the comment docs.

@codecov
Copy link

codecov bot commented Sep 3, 2019

Codecov Report

Merging #52 into master will increase coverage by 0.18%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #52      +/-   ##
==========================================
+ Coverage   97.55%   97.74%   +0.18%     
==========================================
  Files          42       43       +1     
  Lines        1679     1727      +48     
==========================================
+ Hits         1638     1688      +50     
+ Misses         41       39       -2
Impacted Files Coverage Δ
...enCombine/Publishers/Publishers.ReplaceError.swift 100% <0%> (ø)
...rces/OpenCombine/Publishers/Publishers.Print.swift 98.36% <0%> (+3.27%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9b9915b...95fd4ed. Read the comment docs.

@broadwaylamb
Copy link
Member

@beastgrim sure.

This is used for accessing the object that has its property wrapped in @Published. You can find how it can be used here.

You may wonder why I said it's non-trivial. The answer is that you will somehow need to provide a default implementation for the objectWillChange property of the ObservableObject protocol. According to Apple engineers, that requires asking the Swift runtime for class metadata.

Sources/OpenCombine/Published.swift Outdated Show resolved Hide resolved
Sources/OpenCombine/Published.swift Outdated Show resolved Hide resolved
@broadwaylamb
Copy link
Member

broadwaylamb commented Sep 6, 2019

Thanks! This looks good to me. Are you up to write some tests? I could write them myself if you don’t feel like it.

We also have our CI failing, but that’s not your fault, it just needs to be updated for the newest Swift version.

@beastgrim
Copy link
Contributor Author

I can write tests for Published but before I would like complete all in Published functionality. There is one property that doesn't exist now "objectWillChange"

▿ Combine.Published<Swift.String>
  - value: "test"
  - publisher: nil
  - objectWillChange: nil

So I'm going make some investigation how and when this "objectWillChange" uses in Combine framework and after that finish code in "Published" struct. And then cover all code with tests.

I also do not mind if you confirm this pull request. And I will make all these things in another pull request. As it suits you best.

@broadwaylamb
Copy link
Member

broadwaylamb commented Sep 6, 2019

I’ve already investigated it and have a prototype implementation :)

This objectWillChange thing is related to the ObservableObject protocol and involves some low-level stuff, so we’d need to implement ObservableObject first. It doesn’t affect the behavior of Published right now anyway, so it will be okay if you write the tests for what’s already done and I’ll merge this PR.

But if you’d really like to try to make it, you can take a look at this library and this doc on type metadata.

@beastgrim
Copy link
Contributor Author

I could not come up with tests for "Published", this is more like a template for tests. Sorry 😐

Copy link
Member

@broadwaylamb broadwaylamb left a comment

Choose a reason for hiding this comment

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

So I've just merged #54, can I ask you to synchronize with the master branch?

Tests/OpenCombineTests/PublishedTests.swift Outdated Show resolved Hide resolved
Tests/OpenCombineTests/PublishedTests.swift Outdated Show resolved Hide resolved
Tests/OpenCombineTests/XCTestManifests.swift Outdated Show resolved Hide resolved
Copy link
Member

@broadwaylamb broadwaylamb left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution, @beastgrim!

@broadwaylamb broadwaylamb merged commit 07c7a98 into OpenCombine:master Sep 7, 2019
@beastgrim beastgrim deleted the Published branch September 7, 2019 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
missing functionality This functionality is present in Apple's Combine but we don't have it yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants