-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
[workspace] Exported RecentWorkspacePathsData
type, fixed typo in field name
#11603
[workspace] Exported RecentWorkspacePathsData
type, fixed typo in field name
#11603
Conversation
Signed-off-by: Akos Kitta <a.kitta@arduino.cc>
Signed-off-by: Akos Kitta <a.kitta@arduino.cc>
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.
Exporting the types makes sense. I'm not sure whether the typo needs to be fixed.
@@ -89,7 +89,7 @@ export class DefaultWorkspaceServer implements WorkspaceServer, BackendApplicati | |||
* Untitled workspaces that are not among the most recent N workspaces will be deleted on start. Increase this number to keep older files, | |||
* lower it to delete stale untitled workspaces more aggressively. | |||
*/ | |||
protected untitledWorkspaceStaleThreshhold = 10; | |||
protected untitledWorkspaceStaleThreshold = 10; |
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.
The type is unfortunate, but perhaps not worth fixing if it counts as a breaking change?
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.
The type is unfortunate, but perhaps not worth fixing if it counts as a breaking change?
- I can drop the commit. Probably the fastest. I initially wanted to open the PR for the type
export
only, then noticed the typo. - I can deprecate the field and introduce a new one. Initially, I did that and wanted to avoid the why do not you rename it? questions, so I changed my mind and renamed it.
- But I am fine also fine leaving the PR as is. There are breaking API changes between patch releases, and this change is documented. I wonder how many extenders subclass the
DefaultWorkspaceServer
to override this field 😊
Let me know. Thanks!
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.
@kittaakos like you said there's a low chance of someone using this API, so fixing the typo now should be fine.
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.
Code LGTM, breaking change is documented.
Thank you for the review 🙏 |
What it does
RecentWorkspacePathsData
type so that extenders can subclass theprotected
methods of theDefaultWorkspaceServer
class with proper typing.How to test
No behavioral changes are expected.
Review checklist
Reminder for reviewers