FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Passed Exception message to the CashDrawerError class by rakeshgunduka · Pull Request #268 · python-escpos/python-escpos · GitHub

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 commented Oct 24, 2017
edited
Loading

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 commented Oct 24, 2017
edited
Loading

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 commented Oct 24, 2017
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

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 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. ;)

Copy link
Copy Markdown
Contributor Author

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

belono commented Oct 25, 2017

Copy link
Copy Markdown
Contributor

Looks great for me ;)
Thanks a lot.

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.

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

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 commented Dec 3, 2017

Copy link
Copy Markdown
Member

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

patkan merged commit 3c3dab9 into python-escpos:development Dec 3, 2017
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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


Back | FazBrowse Home | New Git URL