Allow an ARKLogMessage to be created with an arbitrary creationDate. … #34
Conversation
|
r @dfed |
|
This is useful for testing, where you might want to decouple ARKLogMessages' creation date from the current system time. Also theoretically useful if using ARK to later persist log messages that were actually recorded at a date in the past. |
rhgills
force-pushed
the
rhg/ark-log-message-create-with-date
branch
from
d244583
to
f14b1e0
Compare
| @@ -27,15 +27,23 @@ NS_ASSUME_NONNULL_BEGIN | |||
|
|
|||
| @interface ARKLogMessage : NSObject <NSCopying, NSSecureCoding> | |||
|
|
|||
| /// @warning Do not use this initializer. | |||
| /// @raises NSInternalInconsistencyException This is not a valid initializer. | |||
| - (instancetype)init; | |||
There was a problem hiding this comment.
Why is this necessary? I'm pretty strongly opposed to giving people rope to hang themselves with.
There was a problem hiding this comment.
Ah. I see why you did this now. Just mark init and new as NS_UNAVAILABLE to prevent the need for this grossness
There was a problem hiding this comment.
Purposefully don't want to mark init and new as unavailable so that subclasses of ARKLogMessage can define init as their designated initializer, if they so wish.
There was a problem hiding this comment.
Before you were already hanging yourself by calling init on a log message, silently creating a useless, empty object. Now this makes your likely programming error more obvious.
There was a problem hiding this comment.
yeah, but before I didn't really know what I was doing, and now I do.
I'd say let's do the following: Create an ARKMessage protocol that ARKLogMessage implements. Mark new and init as unavailable, and make everything that currently takes an ARKLogMessage take an id <ARKMessage> instead. That way we don't need to rely on subclassing (which leads to epic grossness), but rather can rely instead on protocol implementation.
There was a problem hiding this comment.
I agree this is a better design, but this is a pretty substantial refactor. I'm just trying to make a targeted incremental change to make this a bit better. Could we have the protocol approach be our plan of record going forwards?
|
Worth noting that for testing I created Basically, I'm not entirely convinced about the necessity of this functionality in the main project. Please sell me on it :) |
| #pragma mark - Properties | ||
| - (instancetype)initWithDate:(NSDate *)date; | ||
| { | ||
| NSString *text = [[[self class] sharedDateFormatter] stringFromDate:date]; |
There was a problem hiding this comment.
This initializer is truly weird. Why would you want an ARKLogMessage that just contained a date in both NSString and NSDate format?
There was a problem hiding this comment.
Don't know! This isn't new! I think this is for the rows in the ARKLogTableView that just display the time.
|
I do mean turning non-ARKLogMessages into ARKLogMessages at a later date, or perhaps backdating a message explaining an event to the original date of occurrence, even if the event can only be identified later. |
|
ARKFakeLogMessage might work for my purpose, but given that ARKLogMessage declares the date property, it seems cleaner to use that ivar instead of subclassing to provide a setter, which can no longer reference the same (private) ivar |
| @@ -27,15 +27,23 @@ NS_ASSUME_NONNULL_BEGIN | |||
|
|
|||
| @interface ARKLogMessage : NSObject <NSCopying, NSSecureCoding> | |||
|
|
|||
| /// @warning Do not use this initializer. | |||
| /// @raises NSInternalInconsistencyException This is not a valid initializer. | |||
| - (instancetype)init NS_UNAVAILABLE; | |||
There was a problem hiding this comment.
we also need to mark +new as unavailable
rhgills
force-pushed
the
rhg/ark-log-message-create-with-date
branch
from
dc3d05c
to
eb19f4f
Compare
|
Merge when green! |
rhgills
force-pushed
the
rhg/ark-log-message-create-with-date
branch
from
eb19f4f
to
7962a0b
Compare
…Use designated initializers.
rhgills
force-pushed
the
rhg/ark-log-message-create-with-date
branch
from
7962a0b
to
f1e7dbc
Compare
…Use designated initializers.