search

mathpaquette / IQFeed.CSharpApiClient

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

Restrict local IP address list to IPv4. #36

Closed mbmurray closed 5 years ago

mbmurray commented 5 years ago

I simply restrict the list of addresses to those in the IPv4 family.

Nucs commented 5 years ago

@mathpaquette, I would suggest to implement it but also include a static property to either force it to ipv4 or keep it automatic like it is now where automatic is the default.

Nucs commented 5 years ago

This is related to issue #35.

mbmurray commented 5 years ago

I made the suggested change and resubmitted the pull request.

mathpaquette commented 5 years ago

guys, dont forget when you submit a PR, you need to provide some tests to back your changes... cant merge the PR as is...

mbmurray commented 5 years ago

I may just have to close the pull request. The class that was modified doesn't currently have any tests written for it. It is a class that directly interacts with the computer system and is pretty much un(unit)testable as written. Refactoring the code to abstract out the interaction points behind an interface seemed to be outside the scope of the initial change.

mbmurray commented 5 years ago

I added a few integration tests.

mbmurray commented 5 years ago

@mathpaquette , are you going to take a look at this? I've made all the requested updates.

mathpaquette commented 5 years ago

Yes sure Michael I’ll take care soon!!

Get Outlook for iOShttps://aka.ms/o0ukef

From: Michael B. Murray notifications@github.com Sent: Tuesday, June 11, 2019 10:58:51 AM To: mathpaquette/IQFeed.CSharpApiClient Cc: Mathieu Paquette; Mention Subject: Re: [mathpaquette/IQFeed.CSharpApiClient] Restrict local IP address list to IPv4. (#36)

@mathpaquettehttps://github.com/mathpaquette , are you going to take a look at this? I've made all the requested updates.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/mathpaquette/IQFeed.CSharpApiClient/pull/36?email_source=notifications&email_token=ABBICN6RBB73S7KWHBVMMZLPZ64SXA5CNFSM4HG63EDKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXNNLRI#issuecomment-500880837, or mute the threadhttps://github.com/notifications/unsubscribe-auth/ABBICN2EOBKXHVKGFKHFUALPZ64SXANCNFSM4HG63EDA.

mathpaquette commented 5 years ago

@mbmurray im gonna to fix that slightly differently. integration tests like this are duplicated. we just need to use once... anyway, thanks for reporting the issue!