-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Add paging to --list-themes
#3239
Add paging to --list-themes
#3239
Conversation
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.
Thanks, looks good. Is it possible to write an integration test for this, I wonder? 🤔
I do not think integration testing is possible here. A real pager cannot be used when testing because that would cause the test to hang and there would be no way to access the piped data. When using a "dummy" pager (e. g |
(Sorry for closing and reopening, I intended to make this a draft but that does not seem possible on the mobile app) Looking at this once more, I think it is kinda hacky to write to an intermediate buffer and to just tell the |
500aa75
to
48569d5
Compare
48569d5
to
5edaa96
Compare
I wouldn't call myself an expert on this area of the code, but when I skim the code over I can't see anything that looks scary. I haven't looked close enough to mark as Approved, but if both of you think this code is the right approach, I don't mind you merging it. If something breaks it can always be fixed afterwards. |
This allows to use
--list-themes
with paging by passing all theme sample outputs to the pager as a whole.Alternative to #3238
(Closes #3238)