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.

Sign up for GitHub

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

Jump to bottom

Fix function signatures to match C APIs #132

Merged
merged 7 commits into from
Oct 29, 2017
Merged

Fix function signatures to match C APIs #132

merged 7 commits into from
Oct 29, 2017

Conversation

opalmer
Copy link
Owner

@opalmer opalmer commented Oct 28, 2017 •
edited
Loading

@opalmer opalmer added this to the 0.5.0 milestone Oct 28, 2017
@opalmer opalmer self-assigned this Oct 28, 2017
@opalmer
Copy link
Owner Author

opalmer commented Oct 28, 2017

@rodrigc still working on this, thanks for the reports. As soon as I saw #130 I started looking over all the functions currently in pywincffi and will fix all I can find before asking for a review.

@codecov-io
Copy link

codecov-io commented Oct 28, 2017 •
edited
Loading

Codecov Report

Merging #132 into master will increase coverage by 0.27%.
The diff coverage is 100%.

@opalmer
Copy link
Owner Author

opalmer commented Oct 28, 2017

Ok so I've looked the library and it looks like the only other function that has ordering issues is CreateProcess.

@opalmer
Copy link
Owner Author

opalmer commented Oct 29, 2017

@rodrigc please take a look. This should fix the argument ordering issues.

rodrigc
rodrigc approved these changes Oct 29, 2017
View reviewed changes
Copy link
Contributor

@rodrigc rodrigc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made the same fixes in my local tree to get things to work, so this looks good.

rodrigc
rodrigc approved these changes Oct 29, 2017
View reviewed changes
Copy link
Contributor

@rodrigc rodrigc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks OK. I made pretty much the same changes in my local copy of pywincffi to get things to work with Twisted.

You may want to add a test to where lpApplicationName is a valid unicode string. Seems all your tests right now it is None, with one test where it is 1.

@opalmer
Merge branch 'master' into fix-arg-ordering
ec116fe
@opalmer
set lpApplicationName in a couple of tests
b6dc4f8 
lpApplicationName usually does not need to be set because
lpCommandLine typically contains the application name.
@opalmer
fixing merge issue
b0d7372
@opalmer
Copy link
Owner Author

opalmer commented Oct 29, 2017

Thanks, fixed.

rodrigc
rodrigc approved these changes Oct 29, 2017
View reviewed changes
Copy link
Contributor

@rodrigc rodrigc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

@opalmer opalmer merged commit 7b08651 into master Oct 29, 2017
@opalmer opalmer deleted the fix-arg-ordering branch October 29, 2017 19:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rodrigc rodrigc rodrigc approved these changes

Assignees

@opalmer opalmer

Labels
bug lib: kernel32 required-for: twisted
Projects
None yet
Milestone
0.5.0
Development

Successfully merging this pull request may close these issues.
3 participants
@opalmer @codecov-io @rodrigc