-
-
Notifications
You must be signed in to change notification settings - Fork 7.4k
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
feat: new type - DictOrDictList #22387
Conversation
build/csharpTranspiler.ts
Outdated
@@ -330,6 +330,7 @@ class NewTranspiler { | |||
} | |||
|
|||
const csharpReplacements = { | |||
'DictOrDictList': 'List<Dictionary<string, T>>', |
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.
A comment on this stackOverflow post said that this would work
It doesn't look like the transpiler is working though?
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 type does not make sense, have you tested it?
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 type does not make sense, have you tested it?
I updated the T
to object
, but it doesn't get transpiled anyway, so running npm run test
won't test it out, if I insert it in manually no errors show up in my vscode
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.
@carlosmiei Are you able to see why the type is not being transpiled? Into C# and Python?
@@ -5783,7 +5783,7 @@ export default class Exchange { | |||
return this.filterByArray (results, 'symbol', symbols); | |||
} | |||
|
|||
parseTickers (tickers, symbols: string[] = undefined, params = {}): Dictionary<Ticker> { | |||
parseTickers (tickers: DictOrDictList, symbols: string[] = undefined, params = {}): Dictionary<Ticker> { |
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.
again, I think the approach needs to be different here; instead of introducing these exotic types, why not accept only a List and update the implementations providing a dict?
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.
again, I think the approach needs to be different here; instead of introducing these exotic types, why not accept only a List and update the implementations providing a dict?
so that we would have 2 different parseTickers
methods?
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.
or we can just convert from dict to list before calling parseTickers
inside the exchange-specific implementation (I think most exchanges provide a list here)
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.
or we can just convert from dict to list before calling
parseTickers
inside the exchange-specific implementation (I think most exchanges provide a list here)
That's a lot more code, why not just create a type like this, this type will be used for basically all the parsers
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.
or maybe we could have a type like DictOrList
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.
@samgermain because that will not age well and it does not make the code much more strict, it's easier to maintain a method that can only receive a list or a dict and not both, simpler is better in this case
parseTicker
, which I edited in this PR)For some reason the python isn't transpiling