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

Improve error message. #537

Closed
wants to merge 1 commit into from
Closed

Improve error message. #537

wants to merge 1 commit into from

Conversation

BracketMaster
Copy link

@BracketMaster BracketMaster commented Nov 7, 2020

I run into this error every so often and thought
this could be a more helpful error message.

I run into this error every so often an thought
this could be a more helpful error message.
@BracketMaster
Copy link
Author

Assuming anyone else runs into this...

@BracketMaster
Copy link
Author

Or maybe it could say Expected Module not {!r}?

@whitequark
Copy link
Member

There's a warning (just below your change) that says the exact same thing, but also points to the elaborate function in question which I think should be more useful. Does it not fire for you?

@BracketMaster
Copy link
Author

Oh. You're right. My fault for not noticing that.

@whitequark
Copy link
Member

I think I should do something with that warning, if people don't notice it, it's as good as not having it in first place...

@BracketMaster
Copy link
Author

Maybe making it yellow?

@whitequark
Copy link
Member

That's... hard to do in a good way. Think of the people with white terminals where it'll be unreadable.

@BracketMaster
Copy link
Author

don't forget, you can change block backgrounds per character.

@whitequark
Copy link
Member

Right. That will then break on Windows if I use direct escape sequences. And if I want to do it portably it'll be a new dependency and people would complain about that. Colors aren't a good solution.

@BracketMaster
Copy link
Author

This is tricky...
I'm honestly surprised that Windows users use nMigen in
Windows-Python native, instead of just using Bash-for-Windows.

Is red not a good color? GCC and clang always seem to
emit red regardless of the color scheme. And honestly...
if you have a red terminal background, you're asking for
trouble...

@whitequark
Copy link
Member

I'm honestly surprised that Windows users use nMigen in Windows-Python native, instead of just using Bash-for-Windows.

One thing I've learned is that no matter how liberal my assumptions about the downstream environment are, they are always insufficient to cover the workflow of a surprising amount of people. I think every nMigen tutorial that exists describes a completely different installation method, and that's just on Linux!

@whitequark
Copy link
Member

whitequark commented Nov 8, 2020

Also, Windows Store has an official Python package, so if you just type python in cmd (without having anything at all installed) it prompts you to install that. That package actually works pretty well, so I'm happy to have people follow that path.

@BracketMaster
Copy link
Author

You can also just ask nMigen users to read errors
more carefully. Something I should do more often
in general, ha

@BracketMaster
Copy link
Author

I think I've also seen error messages prefixed with
a sign like ==> error foo on line 1234.

@mithro
Copy link

mithro commented Nov 8, 2020

Here is a horrible suggestion (but /might/ work);
(a) Collect all warnings as they are emitted.
(b) On exit, output a summary of the warnings that were emitted (maybe things like the number of each type of warning).
(c) If a traceback / error occurs, make sure to remit the warnings just before the trace back output.

I would generally say that anything which isn't the in the last few lines of output is going to be very easily ignored?

Maybe you could also having a -Werror type thing that is enabled by default and people can turn off if needed?

@whitequark
Copy link
Member

(c) If a traceback / error occurs, make sure to remit the warnings just before the trace back output.

Just before (i.e. above traceback) or just after (below traceback)?

@BracketMaster
Copy link
Author

Just to confirm, I was ignoring what was above traceback
as I assumed it was part of the traceback.

UserWarning was above traceback.

So collecting and going below might be better.

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

Successfully merging this pull request may close these issues.

3 participants