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

Passed Exception message to the CashDrawerError class #268

Merged
merged 7 commits into from Dec 3, 2017

Conversation

rakeshgunduka
Copy link
Contributor

@rakeshgunduka rakeshgunduka commented Oct 24, 2017

Contributor checklist

  • I have read the CONTRIBUTING.rst
  • I have tested my contribution on these devices:
  • e.g. Epson TM-T88II
  • My contribution is ready to be merged as is

Description

@belono
Copy link
Contributor

belono commented Oct 24, 2017

Hi @rakeshgunduka.
I suggest to catch a TypeError in place of the general Exception to solve #267 .

TypeError is the exception raised for non int args:

self.Epson._raw(_CASH_DRAWER('string1', 'string2', 'string3'))
_CASH_DRAWER = lambda esc, p, m, t1=50, t2=50: six.int2byte(esc) + six.int2byte(p) + six.int2byte(m) + six.int2byte(t1) + six.int2byte(t2)
TypeError: an integer is required

and for wrong number of args:

self.Epson._raw(_CASH_DRAWER(27, 112, 0, 64, 54, 54))
TypeError: <lambda>() takes at most 5 arguments (6 given)

also for both wrong number of non int args:

self.Epson._raw(_CASH_DRAWER('string'))
TypeError: <lambda>() takes at least 3 arguments (1 given)

@rakeshgunduka
Copy link
Contributor Author

rakeshgunduka commented Oct 24, 2017

Hi @belono ,
Thanks for the explanation. I am on it.

@rakeshgunduka
Copy link
Contributor Author

Hi @belono ,
Added the TypeError Exception in the except.
Please let me know if anything more need to be updated.

@belono
Copy link
Contributor

belono commented Oct 25, 2017

Great!
Just one little thing.
In python, descriptive variable names are preferred, so, if you don't mind, you could rename the variable e as error or similar. ;)

@rakeshgunduka
Copy link
Contributor Author

Yeah renamed the aliasing as 'err' which is short and descriptive.

@belono
Copy link
Contributor

belono commented Oct 25, 2017

Looks great for me ;)
Thanks a lot.

@patkan
Copy link
Member

patkan commented Oct 27, 2017

Hi @rakeshgunduka thanks for the PR!
Could you please update the authors-file according to https://github.com/python-escpos/python-escpos/blob/development/CONTRIBUTING.rst#author-list? This is why the integration check is currently failing.

@rakeshgunduka
Copy link
Contributor Author

Hi @belono ,
Some checks are still unsuccessful, can you give me a hint where is it failing. Or is it something which can be ignored?

@patkan
Copy link
Member

patkan commented Oct 28, 2017

This test indicates that you have no test-coverage for this functionality. In order to get this away you could write a test, which provokes this error and then checks whether this exception gets raised. A test that does something similar is in https://github.com/python-escpos/python-escpos/blob/development/test/test_function_barcode.py#L35-L36

@rakeshgunduka
Copy link
Contributor Author

Hi @belono ,
This may take some time. I am having some problems in setting up and run the test cases.

@patkan
Copy link
Member

patkan commented Dec 3, 2017

I have added a little test to the PR.
Thank you!

@patkan patkan merged commit 3c3dab9 into python-escpos:development Dec 3, 2017
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.

None yet

3 participants