Closed anntzer closed 1 year ago
Thanks!
What do you think about printing multiline matches by default, instead of requiring an extra option? It seems like a good idea if each match is just a few lines, and a bad one if it's 100 lines (especially if matches overlap!).
Maybe a neater way to control it is to have a configurable threshold for how many lines to print per match, and do some sort of ellipsis for longer matches:
34│ if path.endswith('.so'):
└ <15 lines in match>
Then you could re-run with --max-lines 15
if you wanted to see the full contents of that match.
That seems fine to me, but note that this is only available on Py3.8, so you'll have to choose between restricting astsearch to only work on py3.8, or having different default behaviors on py<3.8 (print single line) and >=3.8 (print whole match). (As the PR is currently written, the effect is that the -m
flag is only present on py3.8+.)
I don't have a strong opinion either way, so you get to pick.
Kindly bumping?
Sorry for the long delay!
In this case, I think the best option is to let the behaviour differ by the version of Python in use, so newer versions of Python get the (hopefully) better feature, and older versions keep the current output. I'm not quite ready to drop 3.7 entirely yet (we're still using it at work), but I'm OK with requiring newer versions for improvements.
OK, I implemented and pushed something close to your suggestion in https://github.com/takluyver/astsearch/pull/10#issuecomment-894772114 (except that exceedingly-long matches are still printed but elided after max_lines lines -- I think this behavior makes more sense, but don't feel strongly about it either).
I'm interested in a slight variation on the output have all lines be:
<file>:<linenumber> <code>
Reason being the above format is often recognized by text editor to jump to line.
I'm happy to send a PR that encompass both my and and @anntzer changes, and @takluyver if you don't want to work on publishing a new version I'm happy to do so if you give me publish rights on PyPI.
Thanks, and sorry again about the long delay in looking at this. I'm going to merge this PR as is now; I think it's a clear improvement.
@Carreau feel free to do what you suggest in a separate PR. Fitting with common editor conventions makes sense. It should obviously be behind an option, though, because I don't think that's generally the best format for human eyes.
Other miscellaneous ideas about presenting output:
│
instead of pipe |
to make the margins look neater?'for ? in range(?): ??'
and the code has nested for i
and for j
loops, at present the inner loop is just shown twice. Maybe that's OK? Or maybe we can find a neater way?How should overlapping matches be shown?
Good point. I guess a proper approach may be to first detect overlapping matches (this can probably be done in a "sliding window" approach, keeping track of all not-yet-exited blocks, checking if the next match overlaps with any of them, and if the next match "exited" a block then remove the block from the track list), then printing out the entire overlapping block all at once and replacing the left pipe/vertical line with >
(or unicode arrow or whatnot) at the relevant lines.
Py3.8+ provides Node.end_lineno which is exactly what's needed for the feature. Test e.g. with
astsearch -m 'if ??: ??'
.Closes #6.
I don't have a strong opinion as to how to name the option.