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

Fixed win32 parser for better windows support #9

Merged
merged 2 commits into from
Jun 18, 2019

Conversation

LucaSoldi
Copy link
Contributor

Windows 10 return string from child_process that doesn't contain "\t" characters.
I tested this split function on a Win7/10 and it works like a charm :)

@DylanPiercey
Copy link
Owner

Would you be able to update the tests to add win32 as well?

@LucaSoldi
Copy link
Contributor Author

Ops sorry, now tests are ok 👍

@natterstefan
Copy link
Collaborator

@DylanPiercey looks good to me, what do you think?

@DylanPiercey
Copy link
Owner

I was hoping to get a test that covered the change, but if you both have tested this the. It’s fine by me 🙂

@LucaSoldi
Copy link
Contributor Author

I was hoping to get a test that covered the change, but if you both have tested this the. It’s fine by me 🙂

The test is updated to cover this pull, tell me if I missed something :)

@natterstefan
Copy link
Collaborator

natterstefan commented Apr 16, 2019

@DylanPiercey I was not yet able to test it on windows, but travis says it's still working.

@LucaSoldi Where did you update the test? I can't see any changed test file in your PR. Have you tried it on your win?

@natterstefan
Copy link
Collaborator

natterstefan commented Jun 18, 2019

Hi @DylanPiercey, what do you suggest what we do with this PR? Merge and release a new version?

@DylanPiercey
Copy link
Owner

@natterstefan I'm fine with it being merged. Again I'd like to see a proper test, but it's a bit tricky I suppose to fake with the CI.

@natterstefan
Copy link
Collaborator

natterstefan commented Jun 18, 2019

@DylanPiercey Good idea with the CI. Let's see how we need to handle the mocked IPs in travis differently in the windows case then:

const mockWinHosts = [
'192.168.0.202 00-12-34-56-78-90 dynamic',
'192.168.0.212 00-12-34-56-78-91 dynamic',
'192.168.0.222 00-12-34-56-78-92 dynamic',
'192.168.0.232 00-12-34-56-78-93 dynamic'
]
.

@natterstefan natterstefan merged commit ae47d46 into DylanPiercey:master Jun 18, 2019
@LucaSoldi LucaSoldi deleted the hotfix/windows-support branch July 3, 2019 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants