Skip to content
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

Limit line lengths #245

Closed
wants to merge 5 commits into from
Closed

Conversation

forgottenswitch
Copy link

@forgottenswitch forgottenswitch commented Nov 22, 2016

Possible fix for #129
Python to generate a test:

with open("long_line_test", "w") as f:
    for n in range(1, 10):
        for i in range(1, 100):
            i_z = str(i).zfill(5)
            f.write("_{}:{}_,".format(n, i_z))
        f.write("\n")

@BurntSushi
Copy link
Owner

@forgottenswitch Thanks for the PR! I really appreciate you taking the time to try to fix #129! :-)

Unfortunately, I think there are significant problems with the patch as-is:

  1. In maximum line length #129, I proposed a specification for fixing the issue, but I don't see any evidence that this was considered here. I would much rather start with a simpler solution than jump to something as complex as this.
  2. Doing stuff based on the tty width seems a bit too magical for my taste. (Moreover, I don't know how to do it on Windows, and the patch as-is looks like it won't compile on Windows.)
  3. The printer code has gotten significantly more complicated and I don't understand most of the changes you've made. I can't merge code that I don't understand.

Before continuing, I think we should focus entirely on my first objection, (1), and avoid getting distracted with (2) or (3) until we actually decide that this is what we want.

@BurntSushi
Copy link
Owner

The proper solution would be to make sure each match has even context before and after.
(except left- and right- most).

It seems very complex and error prone to do this for multiple matches on one very long line. Before I even consider supporting something like that, I would absolutely demand a well designed abstraction around it. I'd suggest we iterate on it in the issue tracker, and not move to the PR phase until we have a design that we're both happy with.

I'd also suggest we wait to do this until I get more of #162 done, which I hope to include pulling the printer out of ripgrep proper and into its own crate.

@BurntSushi
Copy link
Owner

I would strongly urge you not to spend more time on this PR until we hammer out a lot more details and a proper specification. This feature is simply too complicated to do otherwise.

@BurntSushi BurntSushi closed this Nov 22, 2016
@forgottenswitch
Copy link
Author

forgottenswitch commented Nov 22, 2016

What this implements instead of even left- and rightmost context,
is try to print as much as possible before the match, and all that fits after.
This should be a mistake.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants