Check type of ports argument to convert functions.
#366
Conversation
Codecov Report
@@ Coverage Diff @@
## master #366 +/- ##
==========================================
- Coverage 82.56% 82.29% -0.27%
==========================================
Files 35 35
Lines 5952 5856 -96
Branches 1211 1210 -1
==========================================
- Hits 4914 4819 -95
Misses 872 872
+ Partials 166 165 -1
Continue to review full report at Codecov.
|
The `ports` argument to the `convert` functions is a frequent hotspot of beginner issues. Check to make sure it is either a list or a tuple, and give an appropriately helpful error message if not.
| @@ -532,6 +532,13 @@ def prepare(self, ports=None, missing_domain=lambda name: ClockDomain(name)): | |||
| if ports is None: | |||
| fragment._propagate_ports(ports=(), all_undef_as_ports=True) | |||
| else: | |||
| # typecheck ports is either list or tuple (#362) | |||
| if not isinstance(ports, tuple) and not isinstance(ports, list): | |||
There was a problem hiding this comment.
This isn't a major issue here, but I prefer to have typechecking code at the very beginning of the function so that it "fails fast" (before any other errors can arise).
There was a problem hiding this comment.
I'd like to keep this here, because it leverages the existing if ports is None: check. Otherwise we're checking for None redundantly (as we'd need that in the typecheck to avoid a false negative). I view this as being at the top of the "ports exists" branch. I don't feel too strongly about this - I'll ultimately do what you say of course - but I wanted to explain the reasoning.
nmigen/hdl/ir.py
Outdated
| # typecheck ports is either list or tuple (#362) | ||
| if not isinstance(ports, tuple) and not isinstance(ports, list): | ||
| msg = "`ports` must be either a list or a tuple, not {!r}"\ | ||
| .format(type(ports).__name__) |
There was a problem hiding this comment.
The nMigen convention is to use repr of the value in the error, not the name of the type. The reason is that it is often easier to recognize where 1024 came from than an abstract int. I would accept a PR that changes every diagnostic to report both, but in absence of that let's consistently use repr of the value.
nmigen/hdl/ir.py
Outdated
| msg = "`ports` must be either a list or a tuple, not {!r}"\ | ||
| .format(type(ports).__name__) | ||
| if isinstance(ports, Value): | ||
| msg += " (did you mean `ports=(<signal>,)`?)" |
There was a problem hiding this comment.
This suggestion should be more specific, since a typo is often not obvious:
| msg += " (did you mean `ports=(<signal>,)`?)" | |
| msg += " (did you mean `ports=(<signal>,)` rather than `ports=(<signal>)`?)" |
nmigen/hdl/ir.py
Outdated
| @@ -532,6 +532,13 @@ def prepare(self, ports=None, missing_domain=lambda name: ClockDomain(name)): | |||
| if ports is None: | |||
| fragment._propagate_ports(ports=(), all_undef_as_ports=True) | |||
| else: | |||
| # typecheck ports is either list or tuple (#362) | |||
There was a problem hiding this comment.
In general, I avoid referencing bug numbers in code comments. Bugs can have a lot of comments in them and it's not always clear which of them apply. If a bug is important enough to mention in a comment, it's important enough to explain inline or at least in commit message (and as a bonus, doesn't require the reader to be online, GitHub available, etc). But in this case, the reference to the bug adds nothing as this kind of typechecking is pervasive in the codebase.
The
portsargument to theconvertfunctions is a frequent hotspot ofbeginner issues. Check to make sure it is either a list or a tuple, and
give an appropriately helpful error message if not.
Resolves #362 .