LiveUI / Awesome

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

Simplify enums using .allCases from Swift 4.2 #24

Closed ghowen closed 5 years ago

ghowen commented 5 years ago

PR in regards to https://github.com/LiveUI/Awesome/issues/21

I am not so familiar with creating pull requests so please let me know if this is OK like this. Thanks.

rafiki270 commented 5 years ago

Looks good but do you think you could do descriptions for all public variables and methods there for autocomplete? If we’re to upgrade a major version then we should improve all of the interface I think

rafiki270 commented 5 years ago

If the interface changes then we up a major version so youguys don’t have to worry about compatibility

ghowen commented 5 years ago

OK, I implemented the changes suggested by @divadretlaw and also solved the bug with the keys with a computed property. Amazing cannot implement CustomStringConvertible though as this triggers an endless loop due to reflection in the computed description property. But that's no big deal.

Being not so experienced with GitHub and creating PRs I did not realize that @divadretlaw already made some code changes that I could have just accepted. So I did the changes myself in my fork which lead to the suggested changes being outdated. Sorry for this, next time.

@rafiki270 The interface is compatible but with deprecation notices. I'll leave versioning up to you guys.

divadretlaw commented 5 years ago

Did some cleanup as the @available stuff in the protocol did not translate into actual deprecations. Upped the Swift version in all targets and some other general cleanup.

rafiki270 commented 5 years ago

Could we please comment on all the public vars and functions if we are raising the new version?

rafiki270 commented 5 years ago

Happy to merge right after

ghowen commented 5 years ago

@rafiki270 Not fully sure what you mean. Can you give an example of what you have in mind? It looks like autocomplete comments have been removed from Xcode some time ago: https://stackoverflow.com/questions/41830473/xcode-8-2-1-not-showing-documentation-description-on-autocomplete/43982094#43982094

rafiki270 commented 5 years ago

@ghowen https://developer.apple.com/library/archive/documentation/Xcode/Reference/xcode_markup_formatting_ref/index.html basic is ///

rafiki270 commented 5 years ago

you can also go before the property or function and do cmd+/

ghowen commented 5 years ago

@rafiki270 Gotcha. I was missing the ///. cmd+/ creates only // on my machine and that did not work as discussed on SO.

rafiki270 commented 5 years ago

sorry, cmd+option+/ ... it will generate even input/output properties on methods, etc ...

ghowen commented 5 years ago

@rafiki270 OK, I hope this does what you meant. I also added another computed property to Amazing and removed CaseIterable from the enums as they get this through Amazing.

BTW: The shortcut to document does not work (at least on some) non US keyboards. But I found it in the menu and changed it locally. Very helpful!

rafiki270 commented 5 years ago

Looks good to me ... @ghowen would you like to merge and do a release? It will be 2.0.0 and please describe the changes made in the release notes :) ... good job man!

ghowen commented 5 years ago

@rafiki270 Thanks. I've never done that on a public repo and honestly I don't know what else needs to be changed for the release to work properly (like version numbers, pod configs, carthage configs etc.). So maybe someone more experienced can do this one and I'll have a close look at all the changes that were needed for the rollout. Sound OK?

rafiki270 commented 5 years ago

@ghowen don't forget to add yourself to the list of contributors on README page! :)

ghowen commented 5 years ago

:) Sure, thanks. What do you need to do to update the pod and carthage lib? I saw that the pod is still on 1.3.0 while the lib was already on 1.4.0.

rafiki270 commented 5 years ago

Also, should you have a read only prop then skip the setter (re fatal error in vars and lets)