Create toolhelp32 snapshot

[TurBoss]

I have a question
How do you determine in line 260 of process.py the data type?

pls let me know if the PR is ok

thx

[TurBoss]

ok I noticed tha i'm missing the return info

[TurBoss]

how or where I should define constats from the MSDN docs?

[codecov-io]

Current coverage is 97.35%

Merging #101 into master will decrease coverage by 1.17%

Powered by Codecov. Last updated by 3c04364...e6f1d17

[opalmer]

nit: one space after //

[TurBoss]

thx for the comments will check

[opalmer]

To answer your questions:

How do you determine in line 260 of process.py the data type?

The return data type in the docs or something else? If it's the return data type you're after the comment I made in the review should help.

how or where I should define constats from the MSDN docs?

Here - pywincffi/core/cdefs/headers/constants.h

Overall the code looks good, thanks! There's only a couple of things left to address I think other than the comments in the review:

• You need to add a couple of tests to cover the code you're adding:
  • I'd write one test to make sure it works with the current process. Just need to make sure you can call the function and that the output is a handle. Be sure you call CloseHandle on handle returned by your function.
  • I'd write another test which tries to pass in an invalid process id (which should cause an exception to be raised)
• Add the constants from the MSDN documentation to pywincffi/core/cdefs/headers/constants.h

[TurBoss]

Python 3.5.1 (v3.5.1:37a07cee5969, Dec  6 2015, 01:38:48) [MSC v.1900 32 bit (Intel)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> from pywincffi.kernel32.process import CreateToolhelp32Snapshot
>>> lol = CreateToolhelp32Snapshot(0x80000000, 0)
>>> lol
<HANDLE 0x170 at 0x1cea470>
>>>

pushing

[TurBoss]

I was wrong i mean parameters like this

TH32CS_INHERIT
0x80000000

[opalmer]

I was wrong i mean parameters like this

Are you asking how you define them in the headers or how to you use them once they are defined?

[TurBoss]

errr yes if I need to define them

[opalmer]

They need to be defined in the constants.h header I referenced up above.

[TurBoss]

ok thx

[TurBoss]

but how I set the value eg 0x8000000?

[opalmer]

The development documentation covers this: https://pywincffi.readthedocs.io/en/0.2.0/dev/functions.html#adding-new-constants

The ... means the value is filled when the module is compiled.

[TurBoss]

thx!

[TurBoss]

are the test correctly writen?
thx

[opalmer]

Looks like this should be TH32CS_SNAPPROCESS.

[opalmer]

One last request which I forgot to mention earlier, could you please make sure to update the changelog under the 'latest' version?

[TurBoss]

sry but i think that this job is to much pro for me...

[opalmer]

There's code in test_terminates_process, which is a few lines above your code, which runs a process and terminates it. Once the process has terminated you could use the pid of the dead process in your test which should be invalid because it no longer exists.

[TurBoss]

Run the test gives me this :

======================================================================
FAIL: test_invalid_process (tests.test_kernel32.test_process.TestCreateToolhelp32Snapshot)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "c:\cygwin64\home\Jauria\Proyectos\pywincffi\tests\test_kernel32\test_process.py", line 259, in test_invalid_process
    CreateToolhelp32Snapshot(library.TH32CS_SNAPPROCESS, 3)
AssertionError: WindowsAPIError not raised

----------------------------------------------------------------------

I have tryed with 3 as pid, its valid invalid?

[opalmer]

Alright...so after a bit of research I can't get it to raise WindowsAPIError even if I create a process, terminate it, and then feed the terminated process's pid to your function. Guess that's the expected behavior. Sorry for misleading you.

Ok, so all that's left for this PR is:

• Remove the test_invalid_process test. In another branch I've started to develop a way to create a mock test for this and I'll update your tests once that merges.
• Edit the latest version under docs/source/changelog.rst. At a minimum, I'd suggest this:

* Added the :func:`pywincffi.kernel32.CreateToolhelp32Snapshot` function in :issue:`101`. 

[TurBoss]

done and thak you so much for your patience :)

[opalmer]

Looks like there are some lint issues to resolve still:

tests/test_kernel32/test_process.py:254:1: W391 blank line at end of file

Looks like you either need to add or remove a blank line from the end of that file. Do that then run test.bat locally to see if it passes.

[opalmer]

Looks good, thanks! Unless you have objections I think this is ready to merge.

[opalmer]

Going to go ahead and merge this so I can get a release going.