-
Notifications
You must be signed in to change notification settings - Fork 1
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
SenecaBatchProcessor Functional Spec #1
base: main
Are you sure you want to change the base?
Conversation
dist/BatchProcessor.d.ts
Outdated
@@ -1,10 +1,12 @@ | |||
type BatchProcessorOptionsFull = { | |||
debug: boolean; | |||
send: any; |
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.
lets add the object structure as well - helps make it a little self documenting
dist/BatchProcessor.d.ts
Outdated
}; | ||
export type BatchProcessorOptions = Partial<BatchProcessorOptionsFull>; | ||
declare function BatchProcessor(this: any, options: BatchProcessorOptionsFull): { | ||
exports: { | ||
process: (seneca: any, ctx: any, out: any) => any; | ||
process: (this: any, seneca: any, ctx: any, out?: any) => Promise<any>; |
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.
what is the this
here?
src/BatchProcessor.ts
Outdated
|
||
type BatchProcessorOptionsFull = { | ||
debug: boolean | ||
send: any |
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.
and also add the object structures here
test/BatchProcessor.test.ts
Outdated
|
||
// console.log(out, state, ctx) | ||
|
||
expect(state).toEqual({ |
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.
move this outside the message
test/BatchProcessor.test.ts
Outdated
|
||
}) | ||
|
||
describe('more complex', () => { |
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.
test needs a better descriptive name
src/BatchProcessor.ts
Outdated
|
||
} | ||
|
||
class Utility { |
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.
don't use classes as function containers - classes should be only for things that are instantiated
src/BatchProcessor.ts
Outdated
let value = (Inks as any).evaluate(msg[key], { out, ctx }, { sep: '~' }) | ||
|
||
if(null != value && Types[type]) { | ||
let validate = Gubu(eval(type)) |
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.
use Gubu.expr - see https://github.com/rjrodger/gubu/blob/main/test/expr.test.ts#L40
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.
in general, avoid eval in library code, or security static analysis tools will complain
src/BatchProcessor.ts
Outdated
validate(value) | ||
} | ||
|
||
if(null != value) { |
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.
unless there is additional logic, prefer ternaries for assignments like this
new_msg[key] = null == value ? msg[key] : value
src/BatchProcessor.ts
Outdated
return new_msg | ||
} | ||
|
||
static async workflowRun(seneca: any, msg: any, config: any, options: any, results: any) { |
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.
let's refactor this as a preprocessing step, so that workflow exposes a single function run
without any conditional logic at runtime
the preprocessing step can then be unit tested by validating the data structure it generates
src/BatchProcessor.ts
Outdated
for(const message_pattern in match) { | ||
let workflow = match[message_pattern] | ||
// console.log(message_pattern, pattern) | ||
if(ALL == message_pattern) { |
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.
check if the patrun pattern ''
(empty string) can be used as an ALL match - avoids need for special case code
No description provided.