search

mathpaquette / IQFeed.CSharpApiClient

IQFeed.CSharpApiClient is fastest and the most well-designed C# DTN IQFeed socket API connector available
MIT License
121 stars 42 forks source link

Protocol 6.1 #62

Closed NichUK closed 3 years ago

NichUK commented 4 years ago

Protocol 6.1 PR as requested. I'm sorry, it's a bit of a monster, but feel free to ask about anything that's not obvious, or point out some terrible coding. :)

I think it hits all the points from IQ's upgrade 6.0 to 6.1 release notes, but I haven't audited that.

Nich

mathpaquette commented 4 years ago

nice thanks for sharing. Looks promising. gimme some time to review.

NichUK commented 4 years ago

Happy to contribute. As I said, there are definitely a couple of things that I rushed. The MarketSummaryMessage currently contains two copies of all the data which is obviously pointless. There's a comment in there with a couple of ideas. Interested to hear your thoughts when you have some time.

NichUK commented 4 years ago

I screwed up the FundamentalMessage with the protocol update. I didn't realise that they'd removed the reserved fields as well as adding new ones, and apparently didn't test that properly. I have a new commit that fixes it correctly.

mathpaquette commented 4 years ago

I screwed up the FundamentalMessage with the protocol update. I didn't realise that they'd removed the reserved fields as well as adding new ones, and apparently didn't test that properly. I have a new commit that fixes it correctly.

did you run all the integration tests?

mathpaquette commented 4 years ago

dont worry, we gonna merge it at some point, Im just busy on other stuff..

NichUK commented 4 years ago

Sorry, I've been busy too. I did run integration tests, but of course, I updated the Fundamental Message test to what I thought was the new 6.1 protocol wire format... but I was wrong. :) I will have another look at everything very shortly.

NichUK commented 4 years ago

OK - I've been through and sorted the problem with FundamentalMessages and MarketSummaryMessages. It should be good to go now.

mathpaquette commented 4 years ago

we need to rebase this one.

mathpaquette commented 3 years ago

@NichUK could you please rebase this PR?

NichUK commented 3 years ago

Sorry, I haven’t been around for a bit. Happy to look at this again.

mathpaquette commented 3 years ago

Sorry, I haven’t been around for a bit. Happy to look at this again.

Hey @NichUK, it would be nice. I've received some comments that this PR is not following existing design patterns but I think this is due to the fact the previously we're supporting different types before going all-in for doubles.

My recommendation would be to start with a rebase and Ill begin a review to see what changes are needed before merging it.

I know you put some effort into it so Ill prefer to prioritize your work because newer protocol support is getting bigger.

THanks, Mathieu

NichUK commented 3 years ago

OK, so this is a no go. Between the removal of the generics, and the fact that we both implemented Dynamic Fields, but differently, rebasing this is just not going to happen. So I've taken an up to date version, and I'm manually cherry-picking the protocol changes on to it. It's not too hard, so shouldn't take too long, and it'll be way cleaner. I'm just doing 6.1 at this stage, and then we can overlay 6.2 on top. Annoyingly, the current IQFeed beta, has a bug in it regarding OptionChains (the response is always in 6.2 dialect, which is different from previous, no matter what protocol you set - I've reported it), so I'll either have to downgrade for a bit, or suffer the annoyance of tests that fail. Shouldn't be long for a fix though, it's a simple issue for them to solve. Give me a day or two, and I'll open a new PR for it.

mathpaquette commented 3 years ago

@NichUK makes sense. thank you!