search

Rightpoint / BonMot

Beautiful, easy attributed strings in Swift
MIT License
3.54k stars 196 forks source link

composed removed [] #426

Closed DevVenusK closed 1 year ago

DevVenusK commented 1 year ago

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

ZevEisenberg commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

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?

DevVenusK commented 1 year ago

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

ZevEisenberg commented 1 year ago

A description of what?

DevVenusK commented 1 year ago

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

DevVenusK commented 1 year ago

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

ZevEisenberg commented 1 year ago

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

DevVenusK commented 1 year ago

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

ZevEisenberg commented 1 year ago

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

DevVenusK commented 1 year ago

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

done!

DevVenusK commented 1 year ago

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

ZevEisenberg commented 1 year ago

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 commented 1 year ago

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

ZevEisenberg commented 1 year ago

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

DevVenusK commented 1 year ago

thx!! 🤩

vpavelRP commented 1 year ago

@DevVenusK @ZevEisenberg THe cocoapods 6.1.2 is published as well