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

Use long when setting REG_QWORD #50063

Merged
merged 3 commits into from Oct 25, 2018

Conversation

Projects
None yet
5 participants
@twangboy
Copy link
Contributor

commented Oct 15, 2018

What does this PR do?

Uses long for REG_QWORD values in Py2

What issues does this PR fix or reference?

#50039

Tests written?

Yes

Commits signed with GPG?

Yes

@salt-jenkins salt-jenkins requested a review from saltstack/team-windows Oct 15, 2018

@twangboy

This comment has been minimized.

Copy link
Contributor Author

commented Oct 15, 2018

Regarding the lint failure... It is suggesting that I use int instead of long in Py2. It has to be long on Py2 for the Windows API call that occurs later on.

@rallytime

This comment has been minimized.

Copy link
Contributor

commented Oct 15, 2018

@twangboy Can you put a pylint disable for that check then? We can't merge this if lint is red. :)

@twangboy twangboy force-pushed the twangboy:fix_50039 branch from d883ecc to 6b4d892 Oct 15, 2018

@lorengordon

This comment has been minimized.

Copy link
Contributor

commented Oct 15, 2018

Conversion guide seems to suggest from builtins import int... Try that yet?

http://python-future.org/compatible_idioms.html#long-integers

@dwoz
Copy link
Contributor

left a comment

@twangboy Lets see if @lorengordon's comment works before merging this.

@twangboy

This comment has been minimized.

Copy link
Contributor Author

commented Oct 16, 2018

@lorengordon Your suggestion requires adding another dependency to Salt. It doesn't work on Py2 by default. You have to pip install future. So, rather than add another dependency, I think I would rather just use this fix.

@lorengordon

This comment has been minimized.

Copy link
Contributor

commented Oct 16, 2018

@twangboy Ahh, that's true. I already use future everywhere I need 2/3 compatibility because it is so darn handy for these things (much like six). Figured you were also. Oh well.

@cachedout

This comment has been minimized.

Copy link
Collaborator

commented Oct 18, 2018

@dwoz Do you want to come back and look at this again?

@dwoz

dwoz approved these changes Oct 23, 2018

@cachedout

This comment has been minimized.

Copy link
Collaborator

commented Oct 23, 2018

We didn't get Windows test results on that run so I've restarted the test suite here.

twangboy added some commits Oct 15, 2018

@twangboy twangboy force-pushed the twangboy:fix_50039 branch from 5f8ef35 to 5fdba7d Oct 24, 2018

@twangboy

This comment has been minimized.

Copy link
Contributor Author

commented Oct 24, 2018

Rebased

@rallytime rallytime merged commit 9ad43f2 into saltstack:2018.3 Oct 25, 2018

5 of 10 checks passed

continuous-integration/jenkins/pr-merge This commit cannot be built
Details
jenkins/pr/py2-windows-2016 The py2-windows-2016 job has failed
Details
jenkins/pr/py3-centos-7 The py3-centos-7 job has failed
Details
jenkins/pr/py3-ubuntu-1604 The py3-ubuntu-1604 job has failed
Details
jenkins/pr/py3-windows-2016 The py3-windows-2016 job has failed
Details
WIP Ready for review
Details
jenkins/pr/docs The docs job has passed
Details
jenkins/pr/lint The lint job has passed
Details
jenkins/pr/py2-centos-7 The py2-centos-7 job has passed
Details
jenkins/pr/py2-ubuntu-1604 The py2-ubuntu-1604 job has passed
Details

@twangboy twangboy deleted the twangboy:fix_50039 branch Oct 25, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.