-
Notifications
You must be signed in to change notification settings - Fork 43
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
Dynamic cleanup+example (WIP) #80
Dynamic cleanup+example (WIP) #80
Conversation
Sync to master
Catch up to project master
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.
looks good. just waiting the tests and we can merge that!
src/IQFeed.CSharpApiClient/Streaming/Level1/Level1DynamicFields.cs
Outdated
Show resolved
Hide resolved
src/IQFeed.CSharpApiClient/Streaming/Level1/Messages/UpdateSummaryDynamicMessage.cs
Outdated
Show resolved
Hide resolved
src/IQFeed.CSharpApiClient/Streaming/Level1/Messages/UpdateSummaryDynamicMessage.cs
Outdated
Show resolved
Hide resolved
@BunkerCoder look also 1 test is failing. https://ci.appveyor.com/project/mathpaquette/iqfeed-csharpapiclient/builds/33518962#L873 |
Something like this is good enough isn’t?
ce9d2c3
From: BunkerCoder <notifications@github.com>
Sent: Monday, June 15, 2020 10:33 AM
To: mathpaquette/IQFeed.CSharpApiClient <IQFeed.CSharpApiClient@noreply.github.com>
Cc: Mathieu Paquette <me@mathpaquette.com>; Comment <comment@noreply.github.com>
Subject: Re: [mathpaquette/IQFeed.CSharpApiClient] Dynamic cleanup+example (WIP) (#80)
@BunkerCoder commented on this pull request.
________________________________
In src/IQFeed.CSharpApiClient/Streaming/Level1/Level1DynamicFields.cs<#80 (comment)>:
@@ -223,7 +225,7 @@ public class Level1DynamicFields
public static Level1DynamicFields Parse(string message, DynamicFieldset[] fields)
{
- var values = message.SplitFeedMessage();
+ var values = message.SplitFeedMessage().Skip(1).ToArray();
message contains the raw feed data. From docs "The first field is the Message ID which will be a capital 'Q' for an Update message, or a capital 'P' for a Summary message. The second field is the symbol being updated."
Did you notice the comments about Symbol required to be first? It's easy enough to code around and make it tolerant. How do you want to handle?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub<#80 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABBICN2G7BKOAMS2VHELKI3RWYWI7ANCNFSM4N5ZLUPA>.
|
Instead of doing this I guess you can do:
replace
for (var i = 0; i < fields.Length; i++)
to:
for (var i = 1; i < fields.Length; i++)
Just to avoid creating 2 arrays.
SplitFeedMessage() == create an array
Skip(1) == create another array.
Thanks.
From: BunkerCoder <notifications@github.com>
Sent: Monday, June 15, 2020 10:33 AM
To: mathpaquette/IQFeed.CSharpApiClient <IQFeed.CSharpApiClient@noreply.github.com>
Cc: Mathieu Paquette <me@mathpaquette.com>; Comment <comment@noreply.github.com>
Subject: Re: [mathpaquette/IQFeed.CSharpApiClient] Dynamic cleanup+example (WIP) (#80)
@BunkerCoder commented on this pull request.
________________________________
In src/IQFeed.CSharpApiClient/Streaming/Level1/Level1DynamicFields.cs<#80 (comment)>:
@@ -223,7 +225,7 @@ public class Level1DynamicFields
public static Level1DynamicFields Parse(string message, DynamicFieldset[] fields)
{
- var values = message.SplitFeedMessage();
+ var values = message.SplitFeedMessage().Skip(1).ToArray();
message contains the raw feed data. From docs "The first field is the Message ID which will be a capital 'Q' for an Update message, or a capital 'P' for a Summary message. The second field is the symbol being updated."
Did you notice the comments about Symbol required to be first? It's easy enough to code around and make it tolerant. How do you want to handle?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub<#80 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABBICN2G7BKOAMS2VHELKI3RWYWI7ANCNFSM4N5ZLUPA>.
|
I think this addresses concerns and issues. If it looks OK, I'll complete the test TODOs. |
src/IQFeed.CSharpApiClient/Streaming/Level1/Handlers/Level1MessageDynamicHandler.cs
Show resolved
Hide resolved
Possibly, maybe done. Thanks again for your help. |
If this looks correct, I'll start on testing (new example looking good). TIA for the review.