Improve a value of errors messages

[zerc]

Hey mates!

Here an example of my request:

api_xero.invoices.save({
    'Contact': {'ContactID': 'f7a2766d-babe-48ad-afdc-cbc77b5132b1'},
    'Type': 'ForBar',
    'LineItems': [
        {'Description': 'Test', 'Quantity': 2.0, 'UnitAmount': 10.0}
    ],
    'Date': '2016-03-16',
    'DueDate': '2016-03-20',
    'LineAmountTypes': 'Inclusive'
})

It's an incorrect and raising a next error:

Traceback (most recent call last):
  File "_xero.py", line 28, in <module>
    'LineAmountTypes': 'Inclusive'
  File "/Users/zero13cool/repos/fastfresh/venv/lib/python2.7/site-packages/xero/manager.py", line 187, in wrapper
    raise XeroBadRequest(response)
xero.exceptions.XeroBadRequest: ValidationException: A validation exception occurred

But actually it's totally not valuable information :)

I think more informative may be using of ValidationErrors for making an error message because this key contains info about what really wrong:

"ValidationErrors": [
        {
          "Message": "Invoice not of valid type for creation"
        }
]

Probably it may be works only for are validation errors thought...

[gavinhodge]

From a quick look through the code, the XeroBadRequest object should have a property errors which makes those messages available.

There is also a property problem which has the first available message - useful when you're only expecting a single error message.

try:
    api_xero.invoices.save({})
except XeroBadRequest as e:
    print e.problem

[zerc]

Thank you for an answer,

yes is correct and the problem property have a message what I'm looking for.

But actually, my proposal was about a default behaviour i.e. about making validation errors messages more useful and less confusing, by my mind.

Because when I first time tried to use this I couldn't quickly understand what's wrong without looking into the library code. So not what I'm expected for the first time anyway :)

Of course, for a production application, I'll catch and handle exceptions and will format those messages to fit my needs. So available properties are also will help here. But, it will be useful if those exceptions will reflect an information for a quickly understanding what's wrong.

Simple example:

>>> {}['a']
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
KeyError: 'a'

It definitely said what's wrong :) Instead of:

KeyError: a key is missing in the dictionary

Of course, this is not a critical issue anyway, but about be more user-friendly.

[mackinnon3540]

I'm having a difficult time handling the errors. For example, when the token has expired, I do get an error, but my code is not catching it properly:

try: ... bills=xero.invoices.filter(Type='ACCPAY', Status='AUTHORISED') ... except XeroUnauthorized as e: ... z=e Traceback (most recent call last): File "", line 2, in File "/home/malcolm/anaconda3/envs/snakes/lib/python3.5/site-packages/xero/basemanager.py", line 198, in wrapper raise XeroUnauthorized(response) xero.exceptions.XeroUnauthorized: The access token has expired During handling of the above exception, another exception occurred: Traceback (most recent call last): File "", line 3, in NameError: name 'XeroUnauthorized' is not defined

Can someone explain why my exception handler is not working? Thanks!

[direvus]

I agree with @xerc. It is a bit rough to expect the caller to go digging around in custom attributes of the exception object in order to extract the necessary information. For a validation exception in particular, the default message "A validation exception has occurred" is not useful. The 'problem' is where the useful information lies.

I can see some value in having this in a separate property, but why not also append it to the message?

I'd be happy to create a PR for same.

[freakboy3742]

@direvus Go right ahead. Please don't take the current state of play as being all by design - sometimes it's just because that was the easiest thing to fall out. If you can provide a PR for improved error handling, that would be great.

[direvus]

My PR #150 was just merged, I think that this issue can now be closed.