-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
dvc/dagascii: Use pager instead of AsciiCanvas._do_draw #2815
Conversation
Was just looking through
|
@xliiv is it a draft/WIP still or ready to review? |
Also, could you please elaborate on how pydoc's pager is different from git's pager? |
It was ready for review. Now it needs some work
Also we can temporary make it opt-in via some kind of switch/config in case it doesn't work like it should? |
@efiop i used a narrow window instead, is it ok? i thought that would be quicker. |
@efiop |
Btw isn't this a bit redundant
|
@xliiv I saw that but I think there are many more things wrong with that whole file. I've discussed it with a few people and we all agree it's our new favourite look-at-this-and-feel-better-about-your-badly-written-code example. |
@efiop I'm tempted to say don't rely on |
@casperdcl could you argue that, i'm just curious of your opinion |
erm... this is what pydoc does.
P.S. |
@casperdcl Great points! Have you seen the git source linked in the original issue? E.g. https://github.com/git/git/blob/v2.24.0/pager.c#L48 What are your thoughts about that? At least pydoc is built-in and maintained by someone else. Also seems like a pretty core feature that we could kinda rely on working on most pythons. |
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 a lot!
Btw, @xliiv, would this work on windows in cmd/powershell/gitbash? Esp with horizontal scrolling. |
@xliiv You could add a few stages to the pipeline π E.g.
Or, you could draw something wide directly and feed it to pydoc directly and see how that shows up π But making the terminal smaller also does totally count π |
@efiop I meant to say we can rely on |
@casperdcl Could you elaborate please? |
@casperdcl Or you mean that the fact that pydoc's pager is all over the place, depending on which tool it chooses to use, is a UX concern? |
@efiop thanks for the snippet.
I'll spin up a Windows VM and i'll test it. Todos
|
@casperdcl Regarding the UX. Tried both |
@efiop Are you sure? I tested this approach (and standalone See also So, for now, we have to find a different solution IMO. I'll think of a new approach and i'll put the solution here when find something. |
Though if a submitter rebases to remove/squash/drop extraneous commits then it always looks nicer and easier to track, merge, fix conflicts, and debug. |
Btw @efiop (realted to another discussion somewhere, cc @shcheklein) I don't think it's a problem for maintainers to also do some amount of rebasing before merging. I'm against outright squash-merges in most cases though. |
@casperdcl We are not doing squashes anymore. The current consensus is to format nicely if you can, but we don't require any specific formatting from outside contributors. And in both cases everything gets simply merged, without squashes. |
@xliiv Also note at the tests failed:
|
once build is successful and i have approval, i'll squash commits |
@xliiv FYI: tests failed. |
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! π
Oops, sorry about that, merged as-is. |
Glad could do it :)
everything is ok π |
Uses Stdlib's pydoc to
draw the output in the interactive mode while doing e.g.
dvc pipeline show ..
.Fixes #2807
β Have you followed the guidelines in the Contributing to DVC list?
π Check this box if this PR does not require documentation updates, or if it does and you have created a separate PR in dvc.org with such updates (or at least opened an issue about it in that repo). Please link below to your PR (or issue) in the dvc.org repo.
β Have you checked DeepSource, CodeClimate, and other sanity checks below? We consider their findings recommendatory and don't expect everything to be addresses. Please review them carefully and fix those that actually improve code or fix bugs.
Thank you for the contribution - we'll try to review it as soon as possible. π
Related MR:
iterative/dvc.org#831