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

Include type name in TypedDataType construction errors #6015

Merged
merged 2 commits into from Jun 26, 2018

Conversation

Projects
None yet
4 participants
@illicitonion
Copy link
Contributor

illicitonion commented Jun 25, 2018

No description provided.

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor

cosmicexplorer commented Jun 25, 2018

This is a great and useful change! Thanks for noticing and addressing this!

I would recommend:

  1. fixing the FIXME I left later in the file by making TypedDatatypeInstanceConstructionError subclass TypeError (and then fixing any tests, or looking for tests that catch Exception and make them catch a TypeError instead — not clear if those exist (update: looks like they do not!)),
  2. then use the classmethod make_type_error() (not necessary, but consistent error messages are neat and one way to do that is to use a common method),
  3. and I would probably want to add the exception e as the second argument to the re-raised TypeError so that the full stack trace is preserved (I think that’s how it works?? make_type_error() should forward the argument to the TypeError ctor correctly).

Alternatively, if any of the above seems totally unnecessary, feel free to exterminate or improve it. Also let me know if any of the above isn’t clear.

@dotordogh
Copy link
Contributor

dotordogh left a comment

Looks great to me! Thank you for doing this!!

@illicitonion illicitonion merged commit 6d2cf04 into pantsbuild:master Jun 26, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@illicitonion illicitonion deleted the twitter:dwagnerhall/typeddatatype/errors branch Jun 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment