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

composed removed [] #426

Merged
merged 5 commits into from
Feb 7, 2023
Merged

composed removed [] #426

merged 5 commits into from
Feb 7, 2023

Conversation

DevVenusK
Copy link
Contributor

[] can be removed when using the composed method.
Overloading methods for optional use.

[] can be removed when using the composed method.
Overloading methods for optional use.
@ZevEisenberg
Copy link
Collaborator

@DevVenusK thanks for your contribution! I don't fully understand the issue that this fixes. Would you mind posting a reproduction case? For bonus points, and unit test would be great as well.

@DevVenusK
Copy link
Contributor Author

DevVenusK commented Jan 16, 2023

@ZevEisenberg Currently we have to use it like this to use the compose method.
.composed(of: [ "apple".styled(...), "is delicious".styled(...) ])

However, if you use the Variadic Parameters, you can do this.
.composed(of: "apple".styled(...), "is delicious".styled(...) )

I just changed the parameter :)
I really like the missing [].

@ZevEisenberg
Copy link
Collaborator

Ah, now I understand. Can you perhaps simplify the code by calling through to an existing function, passing along the array from the variadic parameters, instead of duplicating the whole function body?

Simplify the code by calling through to an existing function.
Writing Description.
@DevVenusK
Copy link
Contributor Author

good idea! I fixed it!
Would you mind posting a description?

@ZevEisenberg
Copy link
Collaborator

A description of what?

@DevVenusK
Copy link
Contributor Author

Sorry, I meant documentation comments, not a description. Is it okay to use the documentation comments of this method as is?

Copy link
Collaborator

@ZevEisenberg ZevEisenberg left a comment

Choose a reason for hiding this comment

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

If the comment is copy-and-pasted, it should be good, with the exception of any mention of an array, which should probably be changed to "list"

Sources/Composable.swift Outdated Show resolved Hide resolved
@DevVenusK
Copy link
Contributor Author

Oh, I hadn't thought of that thanks. Edited.

@ZevEisenberg
Copy link
Collaborator

Looking good! I'd still like to see a unit test to make sure we don't break this syntax in future.

@DevVenusK
Copy link
Contributor Author

Write unit test for variadic parameter. 🙇🏻‍♂️

Tests/ComposableTests.swift Outdated Show resolved Hide resolved
@ZevEisenberg
Copy link
Collaborator

Nice test! Let's get the whitespace cleaned up and then we're good to merge.

@DevVenusK
Copy link
Contributor Author

Nice test! Let's get the whitespace cleaned up and then we're good to merge.

done!

@DevVenusK
Copy link
Contributor Author

@ZevEisenberg Do you have any idea when this PR will be merged?

@ZevEisenberg
Copy link
Collaborator

Sorry, I've been procrastinating on it because I don't know if our CocoaPods release process works any more. I'll see what I can do.

@ZevEisenberg ZevEisenberg merged commit 3989599 into Rightpoint:master Feb 7, 2023
@ZevEisenberg
Copy link
Collaborator

Merged, but having trouble with the CocoaPods release, as anticipated. Working on it.

@ZevEisenberg
Copy link
Collaborator

But you can use it with SPM now because the tag is pushed.

@DevVenusK
Copy link
Contributor Author

thx!! 🤩

@DevVenusK DevVenusK deleted the patch-1 branch February 16, 2023 09:20
@vpavelRP
Copy link

@DevVenusK @ZevEisenberg THe cocoapods 6.1.2 is published as well

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.

4 participants