Skip to content

Passed Exception message to the CashDrawerError class#268

Merged
patkan merged 7 commits into
python-escpos:developmentfrom
rakeshgunduka:development
Dec 3, 2017
Merged

Passed Exception message to the CashDrawerError class#268
patkan merged 7 commits into
python-escpos:developmentfrom
rakeshgunduka:development

Conversation

@rakeshgunduka

@rakeshgunduka rakeshgunduka commented Oct 24, 2017

Copy link
Copy Markdown
Contributor

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

belono commented Oct 24, 2017

Copy link
Copy Markdown
Contributor

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

rakeshgunduka commented Oct 24, 2017

Copy link
Copy Markdown
Contributor Author

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

@rakeshgunduka

Copy link
Copy Markdown
Contributor Author

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

@belono

belono commented Oct 25, 2017

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Contributor Author

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

@belono

belono commented Oct 25, 2017

Copy link
Copy Markdown
Contributor

Looks great for me ;)
Thanks a lot.

@patkan

patkan commented Oct 27, 2017

Copy link
Copy Markdown
Member

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
Copy Markdown
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

patkan commented Oct 28, 2017

Copy link
Copy Markdown
Member

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
Copy Markdown
Contributor Author

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

@patkan

patkan commented Dec 3, 2017

Copy link
Copy Markdown
Member

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.

3 participants