-
Notifications
You must be signed in to change notification settings - Fork 30
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
GLSP-1393 Align Interface usage across *Manager classes #388
Conversation
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.
Thank you very much Nina! The code looks good to me in general, I just have some worries regarding the injection - which in my opinion should use the interfaces.
@tortmayr Are you happy with the general split between type/interface/class?
import { inject, injectable, optional } from 'inversify'; | ||
|
||
@injectable() | ||
export class WorkflowStartup implements IDiagramStartup { | ||
rank = -1; | ||
|
||
@inject(GridManager) @optional() protected gridManager?: GridManager; | ||
@inject(TYPES.IGridManager) @optional() protected gridManager?: GridManager; |
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.
@inject(TYPES.IGridManager) @optional() protected gridManager?: GridManager; | |
@inject(TYPES.IGridManager) @optional() protected gridManager?: IGridManager; |
If we are already introducing interfaces for the manager classes, we should only rely on those interfaces when injecting that class. This is true for all injection points for all manager classes.
In general yes. I think this is the best solution to give adopters full flexibility for customizations. We just have to make sure that we keep interface and default implementation in sync if we make future changes. i.e. every public method/field of the default implementation needs to already be added to the interface. I think this is a pattern that we could extend to our other services as well. (e.g. the selection service). |
Thanks for the feedback! I'll address it in the course of the upcoming week! |
- Align interface + symbol usage for *Manager classes - `ChangeBoundsManager`, `GridManager`, `DebugManager` - Use bindAsService to ensure backward compatibility - Update injects to use the interface instead of the actual manager class Resolves eclipse-glsp/glsp#1393
6ff6dfb
to
335a49f
Compare
Thank you both for your comments. I’ve updated my commit and created a follow-up issue (GH-1400) to apply the same pattern to other services, as Tobias suggested. |
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.
Thank you, Nina! Everything still looks good to me!
- Fix manager changes: eclipse-glsp/glsp-client#388
…#388) - Align interface + symbol usage for *Manager classes - `ChangeBoundsManager`, `GridManager`, `DebugManager` - Use bindAsService to ensure backward compatibility - Update injects to use the interface instead of the actual manager class Resolves eclipse-glsp/glsp#1393
…#388) - Align interface + symbol usage for *Manager classes - `ChangeBoundsManager`, `GridManager`, `DebugManager` - Use bindAsService to ensure backward compatibility - Update injects to use the interface instead of the actual manager class Resolves eclipse-glsp/glsp#1393
What it does
ChangeBoundsManager
,GridManager
,DebugManager
Resolves eclipse-glsp/glsp#1393
How to test
Changing bounds, grid toggling and debug mode toggling should work as before, this should not impact the functionality at all.
Follow-ups
--
Changelog