-
Notifications
You must be signed in to change notification settings - Fork 5
correlation is now created by first FS in chain #109
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.
need to add correlation argument to contract test, as this feature will now be part of the FS contract
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.
Same comment as Amir has on contract test.
Also maybe to consider making correlation value assignment with argument default or use other variable name to do not spoil original argument.
src/memory-fs.ts
Outdated
if (this.isIgnored(fullPath)) { | ||
throw new Error(`Unable to save ignored path: '${fullPath}'`); | ||
} | ||
correlation = correlation || makeCorrelationId(); |
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.
isn't this better as default argument.
saveFileSync(fullPath: string, newContent: string, correlation: Correlation = makeCorrelationId()): Correlation
It maybe not possible due to API interface, but then maybe to use other variable. After that assignment would be hard to check if there were any correlation argument at the first.
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.
done, made some more changes, take a look
src/local-fs.ts
Outdated
const correlation = makeCorrelationId(); | ||
async ensureDirectory(fullPath: string, correlation:Correlation = makeCorrelationId()): Promise<Correlation> { | ||
this.registerCorrelationForPathsInDir(fullPath, correlation, 'directoryCreated') | ||
// this.registerCorrelator(['directoryCreated'], correlation, e => fullPath.startsWith(e.fullPath), true); |
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.
you forgot to remove this
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.
thanks, any idea why the travis C keeps failing? should i ignore or explore?
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.
as far as i see from logs, it can't start headless chrome on linux only, so I would say ignore.
afaik it is also broken on master right now. Maybe @amir-arad knows
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.
sadly it's indeed broken in master. I wo'nt be around to fix it anytime soon, so as long as it fails on loading chrome, it's fine. maybe @AlexShemeshWix can have a go at it?
# Conflicts: # src/wamp-client-fs.ts # test/local-fs.spec.node.ts # test/no-feedback-events-fs.spec.ts
# Conflicts: # src/wamp-client-fs.ts # test/local-fs.spec.node.ts # test/no-feedback-events-fs.spec.ts
# Conflicts: # src/wamp-client-fs.ts # test/local-fs.spec.node.ts # test/no-feedback-events-fs.spec.ts
de-dupe local-fs events
add correlation argument to contract test
No description provided.