-
Notifications
You must be signed in to change notification settings - Fork 1.7k
[py3] Add extra details to the AvroTypeException #306
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
Conversation
|
Hej, can I have any feedback from people interested in maintaining the Python version? |
|
Can we have this for Java as well? |
|
Ha, it would be nice, but unfortunately I have very limited experience with Java. After briefly looking at the code, I think it would be possible to introduce some error aggregator, but I'd need to spend some time understanding how the validation works in Java version of this lib. |
|
To whom it might concern: I have comparable pull request for the PHP library (forked by wikimedia): |
lang/py3/avro/io.py
Outdated
| BLUE = '\033[94m' | ||
| GREEN = '\033[92m' | ||
| RED = '\033[91m' | ||
| ENDC = '\033[0m' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any way we can disable these term colors when stdout is not a tty, or when the termcap doesn't support it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolutely, I think it's enough to check sys.stdout.isatty(). I'll do it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kojiromike I considered using colorama, but finally I decided not to add another dependency and just check if the terminal is tty.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you considered using an extras_require on colorama?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be good idea, but the purpose of colorama would be to clean the stdout from all color strings, so the code could work for ex. on Windows. Then the lib should be mandatory requirement, otherwise Windows users would get some weird strings in output.
Another problem I discovered is that colorama makes the code untestable, because the lib injects different ANSI escape character sequences depending on the OS. I experimented with this solution and found that there is no easy way to unit-test such code.
|
I love it. I totally want to improve these error messages. I have one comment, that we should offer a way to turn term colors off when we need to. |
|
I agree with @kojiromike : I don't think terminal color codes belong in exception messages. |
|
Hey @kojiromike, do you think it's safe to merge this PR? Should I improve something else? |
|
@sireliah Can you resolve the conflicts? |
|
@Fokko Sure, I will do! |
|
Ok, conflicts resolved and all tests are passing. However I see a potential threat of conflicts with @kojiromike pull request here: #516 |
|
@sireliah Can you rebase? |
|
@Fokko In #516 we discussed with @kojiromike possibility to refactor the validation and have the simplified version. I'm working currently on rewriting the validation in such fashion that it is compatible with his changes. Also I noticed that with the ANSI term colors it would be highly adviced to follow standards: https://no-color.org/ https://bixense.com/clicolors/ and check for Ah, also - after a year of merges in the master, rebasing my branch is extremely difficult. : ] |
Hey, I decided to prepare a small improvement to the AvroTypeException and the schema validation in python3.
My use case is following:
recorddata types.intinstead ofstring), the AvroTypeException is raisen and schema is printed to stdout.Solution:
What do you think about the idea? Feedback/review would be really appreciated.