-
Notifications
You must be signed in to change notification settings - Fork 63
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
140/channels filter by connection #153
Conversation
26805c1
to
78f1771
Compare
78f1771
to
4c58c87
Compare
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.
Looking good.
'transfer', | ||
'custom', | ||
Order.ORDER_UNORDERED, | ||
'ics20-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.
Are these values not available from test utils?
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 didn't know but there's something. I had to flip things up but it works: c47a936
|
||
fsReadFileSync.returns(registryYaml); | ||
|
||
await run(options, (logger as unknown) as Logger); |
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.
Hm, maybe we should unify the Logger
types in lib
and binary
.
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.
Yes, I already spotted this here: #149 (comment)
Created a task: #155
.find((value) => | ||
value.match( | ||
new RegExp( | ||
`[^\\s]+\\s+(?!${options.port})[^\\s]+\\s+[^\\s]+\\s+[^\\s]+` |
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.
This is a little hard to understand and seems easy to make a mistake and get false negatives here. How about checking that every channel has the correct port?
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're right. I inverted it and also spotted an edge case with last empty line.
I think it's easier to follow now, see: 281a400
Closes #140