LiveUI / Awesome

FontAwesome 6 Swift & SwiftUI implementation for iOS, tvOS & macOS
MIT License
97 stars 27 forks source link

Make the lib a bit more swifty #21

Closed ghowen closed 5 years ago

ghowen commented 5 years ago

A few suggestions to make the lib even more swifty:

I really appreciate all your work! This is just a suggestion and I'd love to hear your thoughts on this. Thanks.

rafiki270 commented 5 years ago

Sounds interesting, would you be willing to do a PR?

ghowen commented 5 years ago

Love to, but don't have the time at the moment. The description part needs 2 lines of code for each case and I'm right in the middle of a big project. But I can look at it at a later time if no one picked it up by then.

ghowen commented 5 years ago

OK, I looked at it and it's actually more simple as it's just a few lines of code in the generator.

The main question is if you want to preserve backward compatibility. In Swift 4.2 we now have allCases() on an enum that implements the CaseIterable protocol. Just by adhering to the protocol, we could drop the public static var all.

If we decide to break backward compatibility, we could then do a bigger clean up and get also rid of static var keys: [String] and static var labels: [String] { get } as we can now iterate the enum and get the keys and labels directly from the enum.

The Amazing.label would need to be renamed to Amazing.description as defined in the CustomStringConvertible protocol.

Amazing.name could then be renamed to Amazing.key to be more in sync with the key:value concept.

@rafiki270 How do you feel about breaking backward compatibility and the changes?

ghowen commented 5 years ago

@rafiki270 I was also wondering about the reason for

    public static func fromType(_ code: String) -> AwesomeType? 

as IMHO you could just directly call for example

    let amazing = AwesomePro.light(rawValue: "xxx")

which IMHO does the same thing. So if you feel about breaking backward compatibility, you could probably also remove this.

ghowen commented 5 years ago

And while we're at it I think that Swift naming conventions advocate naming enums with a capital letter at the beginning. But while the other changes could be implemented softly, this definitely would break things all over the place.

Or we branch and start version 2.0?

Sorry for all the comments but I had a little time on my hands to look a bit closer and I would love to implement the lib in my current project.

ghowen commented 5 years ago

Happy to do a PR once we decide if we want to break things or not :)

divadretlaw commented 5 years ago

In Swift Enumerations they use lowercase enums. I remember that they started with a capital letter in Swift 2 or 3.

I would not recommend breaking changes, but instead simply mark the obsolete functions and properties with e.g.

 @available(swift, deprecated: 4.2, message: "This will be removed in the future, please migrate to ...")

to let users know that there are alternatives without breaking their code. I don't think we need backwards compatibility with older Swift versions.

ghowen commented 5 years ago

@divadretlaw In the link you provided to Swift Enumerations they use capital letters for the enum itself and small letters for the cases within the enum. That's what I propose, but it would break the current way as the Awesome (capital A) is a struct that encloses an enum solid (lower cased s).

Good idea about the @availabe attribute. We could warn now and remove later. The enums would need to stay lower cased though.

divadretlaw commented 5 years ago

Ah. I thought you were talking about the cases, my bad. In this case @available strikes again by adding

@available(*, deprecated, renamed: "Brand")
public typealias brand = Brand

the code won't be broken but the capital letter enums will be available.

screen shot 2018-10-27 at 16 07 29

ghowen commented 5 years ago

Excellent. I am learning something new every day. Love it!

ghowen commented 5 years ago

https://github.com/LiveUI/Awesome/pull/24