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.
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.
| @@ -532,12 +532,12 @@ 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) | |||
| # typecheck ports is either list or tuple | |||
There was a problem hiding this comment.
Now this comment is just tautological with the code that follows it :)
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 .