Adding swift wrapper for logmanager and logger classes over Objc #1143
Conversation
absaroj
requested review from
ThomsonTan,
bliptec,
kindbe,
lalitb,
maxgolov,
mkoscumb,
reyang and
sid-dahiya
as code owners
wrappers/swift/Logger.swift
Outdated
| // | ||
|
|
||
| /// Wrapper class around ObjC Logger class `ODWLogger` used to events. | ||
| public class Logger { |
There was a problem hiding this comment.
Do we expect subclasses of this class? If not, we should mark this as final to prevent unexpected behaviors if someone chooses to subclass.
There was a problem hiding this comment.
Don't see it now but in future it might get subclassed as it has lots of instance methods. So keeping it non-final.
There was a problem hiding this comment.
The problem is that these classes don't seem designed for subclassing since they are modifying class properties instead of instance properties. The state management can get pretty complicated.
There was a problem hiding this comment.
Not sure if I understand the comment. Can you please explain the class properties vs instance properties? As I see this class is indeed being instantiated and modifying via its instance methods.
There was a problem hiding this comment.
While this class is modifying its internal state via instance variables, there isn't a defined contract on how other classes should subclass this. As a subclasser, will I be required to call super at some point in my implementation? Do I do that at the start or at the end of my implementation? If I override on function, do I need to override the others?
|
There seems some CI failures for iOS build. |
|
Merging to unblock. iOS build failure is not related to this PR. |
Adding swift wrapper for logmanager and logger classes over Objc .