-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Move to ComposableLogsSheetKit #1
Conversation
public func log(isDebug: Bool) -> Self { | ||
.init { state, action, environment in | ||
if isDebug { | ||
state.logs.append(.init(message: "\(action)")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ I think it is better to be able to customize log message(not just action) for the open source
purpose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about adding one more function?
public func log(isDebug: Bool, with message: String) -> Self {
.init { state, action, environment in
if isDebug {
state.logs.append(.init(message: message))
}
return self.run(&state, action, environment)
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a786f19 에 추가 했어요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in that case we cannot use action.
what i mean is like this
public func log(
isDebug: Bool,
withMessage message: @escaping (Action) -> String)
) -> Self {
if isDebug {
state.logs.append(.init(message: message(action)))
}
return self.run(&state, action, environment)
}
then, the user can customize log message using action
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow! I like that way better! 🔥
8cbaf39 에 추가 했어요!
import LogsSheetKit | ||
|
||
public protocol LoggableState: Equatable { | ||
var logs: [ActionLog] { get set } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ Is it okay to have logs without maximum capacity?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean limit size of the logs array?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes! What's your thought about it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds good for me in case of for instance big amount of logs, which then will have to be shown all at once in the LogsSheet. If the amount is too big, it is very unlikely that user will scroll down till the end.
However, I think for the debug reasons, sometimes we want to know the whole path of actions, which might need to see the very first actions as well as very last ones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wow, I totally agree with you that someone may want the start of the log.
Added function documentation as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome 🎉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
고생하셨습니다 👍
LogsSheetKit:
https://github.com/riiid/LogsSheetKit