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
refactor(log): Deprecate BaseHandler.setup()
and BaseHandler.destroy()
#4435
refactor(log): Deprecate BaseHandler.setup()
and BaseHandler.destroy()
#4435
Conversation
BaseHandler.setup()
and BaseHandler.destroy()
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 have a few suggestions. Also, please note that this deprecation is yet to be agreed upon. So it's possible we may not go ahead with it.
@@ -100,11 +114,4 @@ export class FileHandler extends BaseHandler { | |||
#resetBuffer() { | |||
this._pointer = 0; | |||
} | |||
|
|||
override destroy() { |
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.
This method should have a deprecation notice. Otherwise, this is a breaking change. We can also just call .close()
within it now.
} | ||
|
||
override setup() { | ||
open() { | ||
if (this.#isOpen) return; |
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.
This isn't relevant to this PR. I might argue it's unnecessary, as we have the open/close status of this._file
we can use instead.
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.
Can you please revert these changes? This ensures the legacy and possible new methods all work.
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.
Ditto. Can you please revert these changes?
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.
What if we re-orient this PR to just removing BaseHandler.setup()
and BaseHandler.destroy()
? That seems like a more agreeable design decision.
@timreichen, I'm going to close this PR for now. I think we need to discuss and consider this more before proceeding. Either way, thanks for trying to push this forward. |
Ref: #4391
Changes
BaseHandler.setup()
BaseHandler.destroy()
FileHandler.open()
to replaceFileHandler.setup()
FileHandler.close()
to replaceFileHandler.destroy()
setup()
function work withoutBaseHandler.setup()
andBaseHandler.destroy()