-
Notifications
You must be signed in to change notification settings - Fork 2
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
NEW: @W-15652656@: Add public interfaces before adding any business logic #10
Conversation
"branches": 80, | ||
"functions": 80, | ||
"lines": 80, | ||
"statements": 80 |
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.
Lowering per the suggestion of Jag and Josh. But we are still at 100% coverage and I hope to keep it up there as much as possible.
|
||
// Temporarily making this an interface | ||
export interface CodeAnalyzer { | ||
addEnginePlugin(enginePlugin: EnginePlugin): 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.
I'm purposely choosing not to add in any documentation comments since this is all subject to change. Only after things get stable does it make sense for us to add in all the comments for us to manage.
export enum LogLevel { | ||
Error = 1, | ||
Warn = 2, | ||
Info = 3, | ||
Debug = 4, | ||
Fine = 5 | ||
} | ||
|
||
export type LogEvent = { | ||
type: EventType.LogEvent, | ||
timestamp: Date, | ||
logLevel: LogLevel, | ||
message: string | ||
} |
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.
We might be able to reuse some of the Log4JS code that we use in the current scanner repo. Might not have to reinvent the wheel.
getStartColumn(): number | ||
getEndLine(): number | ||
getEndColumn(): number |
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.
Should these three be number|null
?
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.
Will do in my next PR. Sorry I missed your comments.
@@ -0,0 +1,27 @@ | |||
export enum SeverityLevel { | |||
High = 1, |
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.
Did we decide against having a Critical = 0
for engine failures?
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.
Oh that's a good idea. Will update in my next PR.
As a first step, I'm simply adding in all the public interfaces so that we are ready at any time to publish a 0.1.0-alpha package of the core module so that client development may begin in parallel. I went ahead and added in interfaces for both the core package and the engine-api packages.