-
Notifications
You must be signed in to change notification settings - Fork 20
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
Fixing circular dependencies #73
Fixing circular dependencies #73
Conversation
38bfab5
to
97d2230
Compare
97d2230
to
1966e82
Compare
Signed-off-by: Katelyn Nienaber <katelyn.nienaber@broadcom.com>
Signed-off-by: Katelyn Nienaber <katelyn.nienaber@broadcom.com>
…ing extension Signed-off-by: Katelyn Nienaber <katelyn.nienaber@broadcom.com>
Signed-off-by: Katelyn Nienaber <katelyn.nienaber@broadcom.com>
1966e82
to
589ad2f
Compare
Signed-off-by: Katelyn Nienaber <katelyn.nienaber@broadcom.com>
Signed-off-by: Katelyn Nienaber <katelyn.nienaber@broadcom.com>
…tion is used all over the extension) Signed-off-by: Katelyn Nienaber <katelyn.nienaber@broadcom.com>
can you please also update linting to include circular dependency detection? It should look like this. |
Signed-off-by: Katelyn Nienaber <katelyn.nienaber@broadcom.com>
Done! |
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.
Basically there 3 topics of my comments:
- Immutability of objects
- Encapsulation of object properties
- Missing the important properties for logical contract requirements
I suppose, they are not more invasive than interfaces introduction, so maybe we can fix them or they will be just discarded, if we decide to move on with this PR. Anyway, I suppose, this can a be good starting point to discuss the future contracts and how we will treat them.
getStages: () => IStage[]; | ||
} | ||
|
||
export interface IRepository { |
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'm not sure, such repository declaration is valid, isn't it?
The repository itself doesn't make any sense without at least url, name, datasourse. I suppose accessors for such properties should return string literals without any undefined abilities? What do you think 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.
Url, name, and datasource are all private properties of the repo class, therefore they are not included in the interface. We could make them public in the class, but that should be considered during a refactoring of that class, which is out of the scope of this PR.
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.
Yeah, I talked more about getters here. I suppose, the accessor 'getUrl' cannot return undefined and if it return - something went wrong with the entity, because the repository without url doesn't make any sense for me.
Ok, I agree, that the point is in more global scope...
sysName: string | ||
) => IType | undefined; | ||
findFilter: (uri: string) => IEndevorFilter | undefined; | ||
setName: (value: string) => void; |
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.
Why you create setters here?
I think, such domain interfaces can be immutable.
The best advantage of it - working in async flow, you always know, if entity is immutable, that you will get value from accessor even if some promise asynchronously works with the same entity now. What do you think 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.
Again, I think this is out of scope...the setters are existing in the master branch. So if we remove them it should be considered during a refactoring of the whole class, which is not the goal of this PR.
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.
Agree about it
Signed-off-by: Katelyn Nienaber <katelyn.nienaber@broadcom.com>
In this PR:
SubSystem
toSubsystem
everywhere, for continuityModel
toEntities
(because now we have Interfaces folder which serves as a model for most of the app...and Model isn't really a Model)createPathNodes
andcreateEmptyNode
intoEndevorNodes.ts
I think that's everything...lmk what else should be changed :D