-
Notifications
You must be signed in to change notification settings - Fork 33
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
Add Support for Windows #30
Conversation
Codecov Report
@@ Coverage Diff @@
## master #30 +/- ##
==========================================
- Coverage 88.72% 87.29% -1.44%
==========================================
Files 2 2
Lines 417 425 +8
Branches 71 72 +1
==========================================
+ Hits 370 371 +1
- Misses 26 32 +6
- Partials 21 22 +1
Continue to review full report at Codecov.
|
This would be awesome to have! I just have a couple of minor comments on the changes. Thanks! |
This looks good to me. @ryan-pip I assume you were able to give this a test run? I haven't done so due to the elaborate setup required. @ashafer01 do you have any input? If not, I think this is good to merge. |
Yes, i was able to give it a test run. Did a run in a clean env using pip to install and was missing a lib in setup.py but fixed now. |
Two very minor suggestions:
👍 with or without these |
Does anyone have preference one way or another which way we standardise? If we apply the monkey patch in the test and then removed it in the code wouldn’t we end up with a test that would pass and a module that would fail? |
Good point about the test, yeah let's not do that.
|
I also prefer Sounds like we're all on the same page. I'll merge at the end of the day if there are no objections. |
Add windows compatibility by using winkerberos. #28 #22