-
Notifications
You must be signed in to change notification settings - Fork 58
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
build: add DSL for defining platform resources. #59
Conversation
Codecov Report
@@ Coverage Diff @@
## master #59 +/- ##
==========================================
+ Coverage 85.73% 85.91% +0.17%
==========================================
Files 27 28 +1
Lines 4010 4060 +50
Branches 803 817 +14
==========================================
+ Hits 3438 3488 +50
Misses 479 479
Partials 93 93
Continue to review full report at Codecov.
|
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.
See review comments above.
Also, please rename the entire module such that it becomes nmigen.build.dsl
.
Also, please write tests for Pins
and DiffPairs
to verify that these APIs work correctly. I think I will ask for more changes to Subsignal
so you don't need to write tests for that for now.
return "(resource {} {} {} {})".format(self.name, self.number, " ".join(map(repr, self.io)), self.extras) | ||
|
||
|
||
class Connector: |
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.
Should we leave out connectors for now? I think the API here is not well thought out and if we want to get a minimal PoC quickly it would be better to add them later, with more careful consideration.
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.
I agree.
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.
Thank you, this is very good! I thought about Resource
and it is basically OK. Please add a test for that and I'll merge this.
No description provided.