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

Check type of ports argument to convert functions. #366

Merged
merged 4 commits into from Apr 24, 2020

Conversation

awygle
Copy link
Contributor

@awygle awygle commented Apr 24, 2020

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.

Resolves #362 .

@codecov
Copy link

codecov bot commented Apr 24, 2020

Codecov Report

Merging #366 into master will decrease coverage by 0.26%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            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     
Impacted Files Coverage Δ
nmigen/hdl/ir.py 95.45% <100.00%> (+0.03%) ⬆️
nmigen/compat/genlib/fifo.py 72.41% <0.00%> (-4.06%) ⬇️
nmigen/compat/fhdl/structure.py 63.39% <0.00%> (-3.81%) ⬇️
nmigen/compat/fhdl/module.py 72.97% <0.00%> (-2.03%) ⬇️
nmigen/compat/fhdl/bitcontainer.py 75.00% <0.00%> (-1.93%) ⬇️
nmigen/build/plat.py 26.72% <0.00%> (-1.43%) ⬇️
nmigen/_utils.py 79.76% <0.00%> (-0.92%) ⬇️
nmigen/build/run.py 30.37% <0.00%> (-0.88%) ⬇️
nmigen/compat/fhdl/specials.py 31.11% <0.00%> (-0.76%) ⬇️
nmigen/compat/genlib/fsm.py 58.04% <0.00%> (-0.30%) ⬇️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ed0f508...8f7fea8. Read the comment docs.

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 Show resolved Hide resolved
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__)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>,)`?)"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This suggestion should be more specific, since a typo is often not obvious:

Suggested change
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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

nmigen/hdl/ir.py Outdated Show resolved Hide resolved
@whitequark whitequark merged commit f2b4584 into amaranth-lang:master Apr 24, 2020
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.

convert(ports=) is a hotspot of beginner issues
2 participants