-
Notifications
You must be signed in to change notification settings - Fork 73
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
misc: split dialects.utils into multiple files #3472
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3472 +/- ##
==========================================
+ Coverage 90.29% 90.31% +0.02%
==========================================
Files 462 464 +2
Lines 57937 58042 +105
Branches 5566 5562 -4
==========================================
+ Hits 52313 52421 +108
+ Misses 4189 4188 -1
+ Partials 1435 1433 -2 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
@@ -1,6 +1,4 @@ | |||
from abc import ABC | |||
from collections.abc import Iterable, Sequence |
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'm not sure if format
is really the best name to characterise these helpers. Have you thought about further splitting them into printer utils and parsing utils?
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.
These tend to be quite coupled to each other, so it made sense to keep them together. I agree that the name isn't great, but I can't think of a better one... We could also go with the irdl and ir route and ban the sub files of the utils, and expose it all via xdsl.dialects.utils
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'll try that quickly, will result in smaller diff and probably a better experience
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'm not sure I understand the point of the irdl approach, just seems to cause loads of linting errors for me
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.
For some reason, the local development experience is worse than for clients, Pylance automatically inserts the correct import if you pip install xdsl.
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.
sorted this list
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.
Looks good to me!
It feels like a logical split, and leaves space for some upcoming shared dialects helpers.