NEW: @W-15806448@ Implement core run behavior #15
Conversation
| this.emitLogEvent(LogLevel.Debug, getMessage('RunningEngineWithRules', engineName, JSON.stringify(rulesToRun))); | ||
| const engine: engApi.Engine = this.getEngine(engineName); | ||
| const apiEngineRunResults: engApi.EngineRunResults = engine.runRules(rulesToRun, engineRunOptions); | ||
| validateEngineRunResults(engineName, apiEngineRunResults, ruleSelection); |
There was a problem hiding this comment.
Note that I currently have this implemented such it throws an error if something is wrong with what the engine returns. But we could if we wanted to, just emit a log event here with "Error" severity... and then have clients simply inform the user that something bad happened... but continue returning the results even if they are wrong.
But i'm leaning towards just erroring here so that users actually get upset and inform us if we have an engine giving back bad results. That is, I think I'd rather immediately fix an engine and deploy a fix asap (because it really shouldn't happen if we code up the engines well). But i'm open to change this if others disagree. It make sense to change it once we open up the engine api... but right now I propose we keep it as is.
| @@ -0,0 +1,6 @@ | |||
| import path from "node:path"; | |||
|
|
|||
| export function toAbsolutePath(fileOrFolder: string): string { | |||
There was a problem hiding this comment.
In the current scanner, we use a library called normalize-path for this. If it's really just a simple one-liner then we can do it ourselves, but if not we could consider continuing to delegate it.
There was a problem hiding this comment.
As you've probably noticed... I'm trying my best to avoid bringing in unnecessary dependencies that we then need to maintain. Also, normalize-path (https://github.com/jonschlinkert/normalize-path/blob/master/index.js) actually does something different from what I'm doing here.
First, normalize-path always makes the path have forward slashes... which doesn't make sense for windows operating systems that have things like C:\Users\Some\Folder (causing the path to say something like C:/Users/Some/Folder which is invalid).
Second: what my code does is actually fix relative paths to use the correct slash according to the operating system in which it runs and then converts it to an absolute path if it isn't already one.
You'll notice that in some of my test examples I use different slashes to cover it fixing the slashes as needed and that I run my tests both on windows and linux to confirm that things are working.
| @@ -117,4 +119,13 @@ export class RuleSelectionImpl implements RuleSelection { | |||
| getRulesFor(engineName: string): Rule[] { | |||
| return this.ruleMap.get(engineName) || []; | |||
| } | |||
|
|
|||
| getRule(engineName: string, ruleName: string): Rule { | |||
| for (const rule of this.getRulesFor(engineName)) { | |||
There was a problem hiding this comment.
I'm not sure how it would happen, but would the existence of two rules with the same name be an error case as well?
There was a problem hiding this comment.
Yeah good question. I'll add in a validation step when we retrieve the rules from the engine via the describeRules method to ensure that they are unique.
There was a problem hiding this comment.
Added a commit to this PR to add in this validation.
| @@ -131,4 +151,154 @@ function createEngineFromPlugin(enginePlugin: engApi.EnginePluginV1, engineName: | |||
| } catch (err) { | |||
| throw new Error(getMessage('PluginErrorFromCreateEngine', engineName, (err as Error).message), {cause: err}) | |||
| } | |||
| } | |||
|
|
|||
| function extractEngineRunOptions(runOptions: RunOptions): engApi.RunOptions { | |||
There was a problem hiding this comment.
I'd like to see some documentation for this method and the other validation methods. At the very least, a JSDoc header would go a long way.
There was a problem hiding this comment.
I have scheduled to add in doc comments soon as a separate polishing work item. I mentioned this in one of my earlier reviews... but i'm waiting for things to stabilize and finalize before doing so to avoid having to maintain things while things are still changing a bit.
I split this PR into 2 commits.