-
Notifications
You must be signed in to change notification settings - Fork 89
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
Pass strings rather than printing to stdout
in generators
#2061
Conversation
sub_out = self.end_schema(**kwargs) | ||
if sub_out is not None: | ||
out += sub_out | ||
return out |
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.
🎉 🎉 🎉
items.append(self.header(3, "Enums")) | ||
for enum in sorted(self.schema.enums.values(), key=lambda s: s.name): | ||
if enum.in_subset and subset.name in enum.in_subset: | ||
items.append(self.bullet(self.enum_link(enum, use_desc=True), 0)) |
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.
oh one more bugfix - type
-> enum
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2061 +/- ##
==========================================
+ Coverage 79.89% 80.04% +0.14%
==========================================
Files 109 109
Lines 12103 12168 +65
Branches 3449 3449
==========================================
+ Hits 9670 9740 +70
+ Misses 1856 1852 -4
+ Partials 577 576 -1 ☔ View full report in Codecov by Sentry. |
Happy to merge but just want to check one thing. That's a lot of modified snapshots. Looks to be all spurious, I don't think the newline makes much of a difference. But do we anticipate merging this causing a lot of conflicts with any open PRs? |
Yes I should have said I did look through to see and I did not see any substantive changes, and I only kept this change because it seemed like "correct" behavior to not have the extra newlines even if markdown doesnt care. But since markdowngen is destined for deprecation we might also want to minimize change here, so up to yall.
I think any open PRs should be able to merge code and regenerate through any conflict in the snapshots? Or at least that seems like the goal of the snapshotting system. The PR touches a bit of "backbone" code in Generator that doesnt look v high velocity, and it also touches a decent amount of the markdowngen which is similarly low velocity, but in those cases the changes are almost all peripheral/wrapping changes with obvious paths to merge. Any we're worried about in particular? |
Part of: #923
I was sorta like 'this can wait' until i tried to debug something in rdfgen and couldn't use a debugger because stdout was being captured.
design is p self explanatory, only thing that might be weird is that the top-level
Generator
class does simple string concatenation rather than joining with newlines because we assume that the lower level generators will handle newlining (and don't want to prevent them from not doing it if they don't want to).also one bugfix:
__post_init__
- restored thatp much all the changes here are removing extraneous linebreaks that were there bc there wasn't a good way to exlude the newline created by printing
None
. we can restore that if we want a smaller PR idrc :) all the substantive changes are in separate commits to the snapshot regeneration for easier diffing.anyway merry christmas, a gift of labor 2 u