-
Notifications
You must be signed in to change notification settings - Fork 47
Some big changes for a potential Version 4 (windows support and better testing) #71
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
Conversation
Hi there! I like the approach and the idea to launch a version 4. |
setup.cfg
Outdated
norecursedirs = .git venv build dist *egg | ||
addopts = -rfEsxwX --cov readchar | ||
testpaths = tests | ||
addopts = -rfEsxwX -s --cov=readchar |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addopts = -rfEsxwX -s --cov=readchar | |
addopts = -rfEsxwXs --cov=readchar |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the characters following -r
are options for this flag. See the help for pytest:
-r chars
- show extra test summary info as specified by chars: (f)ailed, (E)rror, (s)kipped, (x)failed, (X)passed, (p)assed, (P)assed with output, (a)ll except passed (p/P), or (A)ll. (w)arnings are enabled by default (see --disable-warnings), 'N' can be used to reset the list. (default: 'fE').
Thats why they need to be kept seperate.
Maybe this would make it more clear?:
addopts = -rfEsxwX -s --cov=readchar | |
addopts = -r fEsxwX -s --cov=readchar |
Please, rebase master. I've updated some of the tooling and workflows. |
I will rebase and add your suggstions and update the PR as soon as possible. Thanks for taking a look at it |
@magmax I updated the PR. Take a look when you have the time. |
Through brief testing, the changes made do resolve the issues I was experiencing with version 3.0.5. I second that this should be Pulled into Main and bundled with a new release. |
To fix the linux tests you can use this conftest.py:
We have only 3 failing tests then (I removed all skips)
|
Ah all the
|
very good. I will incorperate this into this PR in the next few days. Most of the Linux code I just copied over from the current version and tried my best to make it a little more dynamic (while having mostly no idea if it is acually a good aproach). So if you have suggestions on how to make this better, feel free to leave them here as well. |
this read_linux.py fixes all tests: I changed line 44, 48, 52 to scan longer scan codes and added 55-57 so we can scan the codes for F5-F10 (they have 5 parts)
|
I also tested the codes via manual input with a short script :D they work now ^^ It's funny that they never worked before :DD but I also only used the Arrow keys and Enter/Space Keys till now. |
Backspace is on linux 0x7f see http://www.macfreek.nl/memory/Backspace_and_Delete_key_reversed |
CTRL_A to CTRL_Z codes are the same on windows as in linux. |
That sounds very good. If you want to you can open a PR on my fork and I can push to this PR from there. That we can also keep diskussing there and stop kluttering this PR :) EDIT: I jsut created a branch on my fork for development, use that because everything I put on master gets directly pushed to this PR |
in |
I think you did the work on this and so it is only fair that you are listed as the commits author :) |
You should add the check for darwin |
good idea and it all the tests seam to pass: https://github.com/Cube707/python-readchar/runs/6272490957?check_suite_focus=true But I would also like some to test it manually a little. Do you have a Mac? I don't and I would like to not have to set up a VM for that... |
Nope I also have no Mac :D |
I just realised that my flatmate has a Mac. I will ask him and test it a bit later. |
Notes for Mac: Most stuff seems to work fine, espacially the arrow keys work. But I found a few keys that don't work:
|
I made this little script for testing. We could maybe ship a clean version of this for manual testing. F5 to F12 also do not work on linux, but Enter works |
Also Ctr+C raises a KeyboardInterrupt, the old version did not. |
The Hex codes for f5 to f12 are wrong, they should be:
then you also have to change line 52 in read_linux
I just tested it:
I can make a PR to your master xD |
On linux it depends on what Enter you press, the Numpad Enter is LF, the normal Enter is CR We probably should also remove ESCAPE_SEQUENCES, I have no idea what use that has. Since they overlap with the Function keys, I think they are wrong |
I also figured that the codes for f5... are probably wrong, but my Linux VM decided to explode yesterday, so I didn't get to testing. I don't have time today and also probably not tomorrow, so if you hsve the timd go ahead and make a PR. |
already done |
fix function keys f5 to f12
I actually removed the testing script, but by now I am considering putting it back. I will at least put it on the dev branch...
That is also deliberate. I feel it is more pythonic that way. When using something like: while True:
c = readkey()
if c == key.UP:
break You have to make sure the user gets not trapped and having to catche the But this is of course open for debate and was just what I felt made the most sense.
interesting and anoying, We will have to wait for userfeedback to see if anyone has problems with that...
I guesse this was an attemped to simplify the readingprocess by looping and comparing against teh list od escape_secenses to now how many bytes to read. But it is no longer needed an I will remove it. |
what was there already a test script xD then I would not have had to make the effort ^^ I would find it handy if you would add something like this in the test folder, you can also quite quickly determine the keycodes for combinations that we do not have in the list, such as Alt+G so all Alt combinations. |
for now I added it under : tests/manual-test.py for final decitions we will have to wait for @magmax anyway... I will also reorder and compress the commits once I get some feedback from him. Then I will probably not delete the testing script and jut update it a little |
btw: I have been thinking about the filestructure and the importprocess a little. Whats your opinion on this:
when typing This than also brings up the question on how to implement mac support. Should it get its own files? even if it just reuses pretty much all of the linux code? Or would be a seperate if line in the init file be clear enough? (maybe also rename linux->unix to make it more clear that its generally used) |
I find my version of the test script better, since it can output all keys from the key definitions. Maybe you can adapt it according to your preferences. You could just make an extra file key.py which will include key_linux or key_python depending on the os. I personally don't need it, but then we're backwards compatible. you can rename linux to posix or unix, i think in python core posix is used more often. |
good idea, I didn't realise you did that. Stolen and incorporated ;) |
closing in favor of #79 |
Hello there,
I really like your project ant wanted to contribute. This is more of a Disskussion PR that should probaply target a development branch, but since I can't create new branches here we go.
addidtions
I cleand up the windows side of the code and made it work consistently with the behavior on the linux side. For that I also had to chage a few small things in the Linux functions.
I than went ahead and upgraded the testing system. For the windows side I creaded a number of testcases covering all printable character and all currently supported special keys (Arrow-keys, F-keys, etc.).
On the Linux side I hit a wall and can only get the tests to work localy, not on GitHub. Maybe someone else can fix that?
I also cleand out the repo, removing (hopefully) not needed files and improving the makefile.
drawbacks
The Linuxside ist still mostly untested and not automated. While I copyed most of the code from the current version, I am not that versed with Linux.
Because windows uses some scancodes that can't be utf-8 encoded directly, I have to do the byte->str conversion manually. This however uses function not present in python 2.7, so I doped that for now. It is probably possible to make this work with 2.7 as well, but I didn't want to waste time on it until I have feedback if this is desirable.
While the libary should be mostly compatible with the current version, there are a few bigger changes that my break compatibilyty with some older projects.