Simplify enums using .allCases from Swift 4.2 #24

ghost · 2018-10-27T17:23:10Z

PR in regards to #21

Simplified enums by removing as many stored properties as possible and replacing them with computed ones. For this mainly the generator and the Awesome.xxx extensions needed to be changed.
Some renaming to be more Swift like but keeping downward compatibility through deprecation warnings. The enums are now capitalized, i.e. FontAwesome.Solid.car instead of FontAwesome.solid.car.
updated enums to FontAwesome 5.4.2

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

Simplified enums by removing as many stored properties as possible and replacing them with computed ones; some renaming to be more Swift like; update to FontAwesome 5.4.2

rafiki270 · 2018-10-27T18:44:16Z

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

divadretlaw

By making Amazing conform to RawRepresentable, CaseIterable, CustomStringConvertible where RawValue == String

we can reduce a lot of the code.

All the new computed properties can be moved into (except public static var all)

extension Amazing {
    ...
}

but the Image / NSAttributedString inits / methods need to be generalized

e.g.

public convenience init(icon: Amazing, size: CGSize, color: Color = Color.black, backgroundColor: Color = Color.clear) {
}

becomes

public convenience init<AmazingType: Amazing>(icon: AmazingType, size: CGSize, color: Color = Color.black, backgroundColor: Color = Color.clear) {
}

Generator/Sources/Generator/EnumBuilder.swift

Classes/Extensions/Amazing+Tools.swift

Generator/Sources/Generator/EnumBuilder.swift

rafiki270 · 2018-10-28T09:28:16Z

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

- Moved most computed properties into Amazing - Updated generator and regenerated enums - Fixed a bug where key and description were not return what was expected - Amazing.name now returns the old key (deprecated), Amazing.key returns the old key prepended by the font name and face

ghost · 2018-10-28T12:01:06Z

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 · 2018-10-28T13:06:45Z

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 · 2018-10-28T13:40:49Z

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

rafiki270 · 2018-10-28T13:41:01Z

Happy to merge right after

ghost · 2018-10-28T13:53:33Z

@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 · 2018-10-29T12:30:08Z

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

rafiki270 · 2018-10-29T12:30:42Z

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

ghost · 2018-10-29T12:56:24Z

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

rafiki270 · 2018-10-29T12:58:02Z

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

- Added documentation to all public interfaces - added .detailedKey and .allDetailedKeys to Amazing that contain a unique key with font name, style and icon name; .key now contains only the lowercased icon name - removed CaseIterable from enums as they inherit this from Amazing

ghost · 2018-10-29T14:42:19Z

@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

There are still missing comments on the new vars unfortunately

rafiki270 · 2018-10-29T15:33:36Z

Classes/Extensions/Amazing+Tools.swift

-        return name
+extension Amazing {
+
+    public var detailedKey: String {


These still missing comments on what they do?

They were already in declaration of the Amazing protocol and I thought this would be enough. But it looks like the properties appear twice in autocomplete: once with (protocol) and once without (extension) the hint. I'll add them to the extension as well.

Looks like the same thing applies for the deprecation warnings. So I'll add them to the protocol again too.

rafiki270 · 2018-10-29T16:09:14Z

Classes/Extensions/Amazing+Tools.swift

-        let name = AwesomeType.keys[index]
-        return name
+
+    @available(swift, deprecated: 4.2, message: "This will be removed in the future. Please use .allKeys instead")


You can also do @available(*, unavailable, renamed: "allKeys") which will offer user the "red dot of fix" in the code ... this is how apple generates their "Fix it!" warnings ...

Yes, but AFAIK this would break downward compatibility as this generates a compiler error instead of a warning. Up to you guys if you want to stay compatible or not.

I would not worry about compatibility as this will be a major update 2.0.0

So if we break downward compatibility, then we could/should also remove the deprecated properties from the protocols and mark them only in the extension as renamed, right?

If it’s an extension of that protocol then yes ... also, the content of that extension method or property should be only fatalError

That's a bit problematic as the computed property expects a return type that fatalError cannot meet. Addind fatalError before the return with the correct type generates compiler warnings that the return will never be executed. IMHO there is no need for the fatalError as the compiler will fail on the unavailable anyway. I'll have another look and submit a proposal.

have you tried? :) ... you need to put it in the get method, set can be let empty ...

var prop: String { get { fatalError() } set { } }

Nice! I it works when using get/set but not when the properties are declared as read-only computed properties.

rafiki270 · 2018-10-30T11:11:43Z

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!

ghost · 2018-10-30T11:18:41Z

@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 · 2018-10-30T11:50:41Z

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

ghost · 2018-10-30T11:52:17Z

:) 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 · 2018-10-30T12:31:12Z

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

Simplify enums using .allCases from Swift 4.2

d83e5cc

Simplified enums by removing as many stored properties as possible and replacing them with computed ones; some renaming to be more Swift like; update to FontAwesome 5.4.2

ghost mentioned this pull request Oct 27, 2018

Make the lib a bit more swifty #21

Closed

divadretlaw requested changes Oct 27, 2018

View reviewed changes

Generator/Sources/Generator/EnumBuilder.swift Outdated Show resolved Hide resolved

Classes/Extensions/Amazing+Tools.swift Outdated Show resolved Hide resolved

Generator/Sources/Generator/EnumBuilder.swift Outdated Show resolved Hide resolved

divadretlaw added 3 commits October 28, 2018 13:59

Make AwesomeFont a CustomStringConvertible

963052f

Cleanup

85d59fe

Fix deprecated AwesomeFont

2d740aa

divadretlaw approved these changes Oct 28, 2018

View reviewed changes

Refactored PoiSelectionTableViewController with Generics

6c18d16

rafiki270 requested changes Oct 29, 2018

View reviewed changes

Georg added 2 commits October 29, 2018 15:54

Added autocomplete hints to Amazing extension

7700061

Added deprecation warnings to Amazing Protocol

3d78a61

rafiki270 reviewed Oct 29, 2018

View reviewed changes

Breaking downward compatibility with compiler hints

34dd167

rafiki270 approved these changes Oct 30, 2018

View reviewed changes

rafiki270 merged commit c737e81 into LiveUI:master Oct 30, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify enums using .allCases from Swift 4.2 #24

Simplify enums using .allCases from Swift 4.2 #24

ghost commented Oct 27, 2018

rafiki270 commented Oct 27, 2018

divadretlaw left a comment

rafiki270 commented Oct 28, 2018

ghost commented Oct 28, 2018

divadretlaw commented Oct 28, 2018

rafiki270 commented Oct 28, 2018

rafiki270 commented Oct 28, 2018

ghost commented Oct 28, 2018

rafiki270 commented Oct 29, 2018

rafiki270 commented Oct 29, 2018

ghost commented Oct 29, 2018

rafiki270 commented Oct 29, 2018

ghost commented Oct 29, 2018

rafiki270 left a comment

rafiki270 Oct 29, 2018

ghost Oct 29, 2018

ghost Oct 29, 2018

rafiki270 Oct 29, 2018

ghost Oct 29, 2018

rafiki270 Oct 29, 2018

ghost Oct 29, 2018

rafiki270 Oct 29, 2018

ghost Oct 30, 2018

rafiki270 Oct 30, 2018 •

edited

Loading

ghost Oct 30, 2018

rafiki270 commented Oct 30, 2018

ghost commented Oct 30, 2018

rafiki270 commented Oct 30, 2018

ghost commented Oct 30, 2018

rafiki270 commented Oct 30, 2018

Simplify enums using .allCases from Swift 4.2 #24

Simplify enums using .allCases from Swift 4.2 #24

Conversation

ghost commented Oct 27, 2018

rafiki270 commented Oct 27, 2018

divadretlaw left a comment

Choose a reason for hiding this comment

rafiki270 commented Oct 28, 2018

ghost commented Oct 28, 2018

divadretlaw commented Oct 28, 2018

rafiki270 commented Oct 28, 2018

rafiki270 commented Oct 28, 2018

ghost commented Oct 28, 2018

rafiki270 commented Oct 29, 2018

rafiki270 commented Oct 29, 2018

ghost commented Oct 29, 2018

rafiki270 commented Oct 29, 2018

ghost commented Oct 29, 2018

rafiki270 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafiki270 Oct 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafiki270 commented Oct 30, 2018

ghost commented Oct 30, 2018

rafiki270 commented Oct 30, 2018

ghost commented Oct 30, 2018

rafiki270 commented Oct 30, 2018

rafiki270 Oct 30, 2018 •

edited

Loading