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

[WIP] Upgrade for python 3 #7

Open
wants to merge 94 commits into
base: master
Choose a base branch
from
Open

Conversation

willprice
Copy link

This PR updates chiplotle for python 3.

See https://github.com/drepetto/chiplotle/blob/13ac28a7645a018a4bc62980cb174627ca6b5b54/chiplotle/tools/hpgltools/pens_updown_to_papr.py

I lifted the code directly from there as the test looked to have been
originally broken, there is an open pull request to fix this issue on
the mainline at drepetto#5
I've ditched unicode literal in the test files that pass src code
strings to `pytest.raises` (they should eventually be upgraded to
context manager style usage) as pytest fails to recognise them as src
code strings, they only do an instance check on `str` objects, and not
unicode:
https://github.com/pytest-dev/pytest/blob/9ef7878cbca555000d88a0ae3abb8cf809e69557/src/_pytest/python_api.py#L627
These were failing because of unicode literals being passed and the
`isinstance(string, type('abc'))` check failing, I've made the change to
support both unicode and bytes which shouldn't cause an issue, although
I'd imagine only ascii strings will be passed - not sure what will
happen if a unicode string that isn't ascii is passed.
willprice and others added 24 commits September 3, 2018 09:34
Copy link
Collaborator

@victoradan victoradan left a comment

Choose a reason for hiding this comment

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

Thanks so much for the update Will! LGTM

tox

compile:
python -m compileall $(LIBRARY_DIR) -j $$(nproc)
Copy link
Collaborator

Choose a reason for hiding this comment

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

nproc not available in mac os... remove?

def test_01():
circ = shapes.circle(100)
filename = "circle"
io.export(circ, filename, "png")
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will fail if hp2xx is not installed. Can we fail gracefully?

@willprice
Copy link
Author

Hi @victordan, thanks for the comments. I'd like to spend some time testing this as I've only just got my HP 7550 up and running and haven't checked I've not introduced any regressions as I've only been able to write virtual tests. I should be able to get this done in the next month or so.

@mjmdavis
Copy link

@willprice how are your updates looking?

@willprice
Copy link
Author

Hi @mjmdavis, I don't have any time to work on this in the forseable future. From what I can recall things were working reasonably, but someone needs to test out all the library functions and make sure it works on a real plotter. I'd only spent time on unit testing.

@schwittlick
Copy link

@willprice is there something in particular to test on the machines? i got some time and also some different plotters around.. please let me know in case you remember. otherwise i'll look around and test it a bit here and there just. thank you!

@willprice
Copy link
Author

Hey Marcel,
It'd be good to have a methodical run through all drawing commands for one plotter, and then testing that the set up routines still work on as many plotters as possible. Thanks for helping out :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants