raise AttributeError in __getattr__ if lookup fails

[vincentschut]

This fixes #88 by raising an AttributeError instead of a KeyError in getattr, which is the more correct thing to do (because hasattr only returns False when it gets an AttributeError, and propagates all other exceptions instead).

[alimanfoo]

Thank you @vincentschut. Would you be able to add a unit test to cover this behaviour?

[vincentschut]

Sure, will try. Though I'm not (yet) fluent in unittests, so beware: noob stupidities ahead! :-)
Would it be sufficient to add a line to test_hierarchy.py:test_getattr() which checks that hasattr does not raise but instead returns False on an non-existing attribute? Or would you rather have a separate, more extensive test?

[alimanfoo]

Thank you, much appreciated. Yes just a simple check that hasattr returns False would be fine. If you want to run unit tests locally you can run the following commands from the repo root directory on your local system (requires Python >=3.4): $ pip install -r requirements.txt $ pip install -r requirements_test.txt $ python setup.py build_ext --inplace $ export PYTHONHASHSEED=42 $ nosetests -v --with-coverage --cover-erase --cover-min-percentage=100 --cover-package=zarr --with-doctest --doctest-options=+NORMALIZE_WHITESPACE zarr Alternatively you can install tox and do: $ tox -e py35 The Travis CI will also automatically run all the tests when you push to the PR, and will only succeed if test coverage is 100%, so if the Travis CI passes you know you're good.

…

On Mon, Dec 5, 2016 at 10:43 AM, Vincent Schut ***@***.***> wrote: Sure, will try. Though I'm not (yet) fluent in unittests, so beware: noob stupidities ahead! :-) Would it be sufficient to add a line to test_hierarchy.py:test_getattr() which checks that hasattr does not raise but instead returns False on an non-existing attribute? Or would you rather have a separate, more extensive test? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <https://github.com/alimanfoo/zarr/pull/95#issuecomment-264821942>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAq8Ql9i19LWwLYUb0bY2Iwat1hzMrsKks5rE-rugaJpZM4LEDnc> .

-- Alistair Miles Head of Epidemiological Informatics Centre for Genomics and Global Health <http://cggh.org> The Wellcome Trust Centre for Human Genetics Roosevelt Drive Oxford OX3 7BN United Kingdom Email: [email protected] Web: http://purl.org/net/aliman Twitter: https://twitter.com/alimanfoo Tel: +44 (0)1865 287721

[vincentschut]

Thank you for these detailed steps, very informative.
I think I nailed it, checks pass both locally and with Travis. Let me know if you expect anything else.

[alimanfoo]

Very minor nit-pick: it would be slightly more direct to use assert_false(hasattr(g1, '__unexistingattribute__')).

[vincentschut]

no problem, changed accordingly.

[alimanfoo]

Thanks for fixing the flake8 error, strange that would only come up now. One minor nit-pick comment on the unit test, otherwise looks good to go.

…

On Mon, Dec 5, 2016 at 11:39 AM, Vincent Schut ***@***.***> wrote: Thank you for these detailed steps, very informative. I think I nailed it, checks pass both locally and with Travis. Let me know if you expect anything else. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <https://github.com/alimanfoo/zarr/pull/95#issuecomment-264833221>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAq8QiJV0drl01MJYmTY-aTihR-KcYMgks5rE_fogaJpZM4LEDnc> .

-- Alistair Miles Head of Epidemiological Informatics Centre for Genomics and Global Health <http://cggh.org> The Wellcome Trust Centre for Human Genetics Roosevelt Drive Oxford OX3 7BN United Kingdom Email: [email protected] Web: http://purl.org/net/aliman Twitter: https://twitter.com/alimanfoo Tel: +44 (0)1865 287721

[alimanfoo]

Great, thank you!