Skip to content
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

Upgrade qdownload to iqfeed 6.2 #7

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Conversation

mpdroog
Copy link

@mpdroog mpdroog commented May 6, 2024

Hello,

I took the liberty to upgrade your qdownload tool from iqfeed protocol 5.1 to the latest 6.2 and add some small changes I thought are useful.

Changes I've done:

  • (protocol) Fix intervalTypes must be lowercase
  • (feature) If no begin or endate set enddate=tomorrow
  • (feature) print config in detailed-logging
  • (protocol) removed requestID (field 0 is now the new messageid LM)
  • (protocol) added tradeaggresor/daycode/microseconds to tickdata

@nhedlund
Copy link
Owner

nhedlund commented May 6, 2024

Thanks for your PR, great upgrading to the latest protocol!

I don't use IQFeed currently so I cannot test your changes unfortunately.

Can you look at the failing tests?

@mpdroog
Copy link
Author

mpdroog commented May 7, 2024

Can you look at the failing tests?
Yes I'll have a look :)

@mpdroog
Copy link
Author

mpdroog commented May 28, 2024

Hi, updated the failed tests, could you approve the workflow unittest?

@nhedlund
Copy link
Owner

I added a couple of questions inline during my review, maybe you did not see them?

@mpdroog
Copy link
Author

mpdroog commented May 28, 2024

Aha didn't see that

@mpdroog
Copy link
Author

mpdroog commented May 28, 2024

Could you hint where I can see your inline questions? Looking at the Files changed shows no comments.

@nhedlund
Copy link
Owner

Wow it seems Github has issues showing inline code comments: https://github.com/orgs/community/discussions/30638

@nhedlund
Copy link
Owner

I am copying the comments here instead:

main.go 181
This worked before so I guess this has to do with the newer protocol?

main.go 216
Previously you did not need to set the end date, you got all data when not setting it. What happens when you don't set the end date? Can you test this?

iqfeed.go 114
If I remember correctly I used request ID to separate multiple parallel requests for different instruments.
Have you tested this change on like for example 20 instruments that download in parallell and verify the data returned that the results are not scrambled (data is allocated to wrong instruments)?

@mpdroog
Copy link
Author

mpdroog commented May 28, 2024

main.go> Thank you, will try collecting the errors I got that motivated me to add this code.

iqfeed.go> I assumed the TCP-connections are separate and thus the data doesn't get scrambled. Will have to more properly test this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants