Skip to content
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

replace overbroad except clauses with if statement and stop shadowing… #6

Merged
merged 3 commits into from
Nov 10, 2016
Merged

replace overbroad except clauses with if statement and stop shadowing… #6

merged 3 commits into from
Nov 10, 2016

Conversation

Eh2406
Copy link
Contributor

@Eh2406 Eh2406 commented Nov 10, 2016

most of a fix for #3

@smmaurer
Copy link
Member

This looks great, thank you!

Can you also add an update to the documentation in README.md to reflect the change in argument names from max -> maximum and min -> minimum?

(For my own benefit, is there a general principle for that? Is it just that the full word is clearer?)

@Eh2406
Copy link
Contributor Author

Eh2406 commented Nov 10, 2016

No it is that min is a built in function of python. So it is easy to make mistakes. if we had

(column_name, str(min(ds)), str(min))

It would have been wrong. Pycharm yelled at me. Doc fix incoming.

@smmaurer
Copy link
Member

Ah, makes sense. Thanks!

@smmaurer smmaurer merged commit f156ec5 into UDST:master Nov 10, 2016
@Eh2406
Copy link
Contributor Author

Eh2406 commented Nov 10, 2016

Do you have advice for the TODO's?

@smmaurer
Copy link
Member

About logging the backtraces? No, i'm not sure how we should handle that.

One of the things i haven't gotten around to is adding Python 3 cross-compatibility, which might give us better options. (I'll open a new issue for that.)

@Eh2406
Copy link
Contributor Author

Eh2406 commented Nov 10, 2016

Or we can use the try but specify the error types we expect. What error do we expect from orca?

@smmaurer
Copy link
Member

I don't know. It could be pretty much anything, because Orca will execute whatever arbitrary code is in the table definition in order to generate it. Seems like ideally we should raise an OrcaAssertionError but also provide the backtrace that triggered it, if possible?

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.

None yet

2 participants