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

Implemented MatchRegexpWithLineNumber() and MatchWithLineNumber() #224

Closed
wants to merge 2 commits into from

Conversation

jessp01
Copy link
Contributor

@jessp01 jessp01 commented Feb 26, 2025

These behave as grep -n does: output the matched line prefixed by the line number.

Simple test:

/tmp/test.txt

Sample data:
This is a test of MatchRegexpWithLineNumber() and MatchWithLineNumber()
They behave as the `grep -n` would
MatchRegexpWithLineNumber() will also find this TEST occurrence because it's case-insensitive

code:

func main() {
        filePath := "/tmp/test.txt"
        pattern := "test"
        str, _ := script.File(filePath).MatchWithLineNumber(pattern).First(3).String()
        fmt.Printf("Testing MatchWithLineNumber with %s...\noutput is:\n%s\n", pattern, str)
        r := regexp.MustCompile("(?i)" + regexp.QuoteMeta(pattern))
        str, _ = script.File(filePath).MatchRegexpWithLineNumber(r).String()
        fmt.Printf("Testing MatchRegexpWithLineNumber with %s...\noutput is:\n%s\n", pattern, str)
}

Output:

Testing MatchWithLineNumber with test...
output is:
2:This is a test of MatchRegexpWithLineNumber() and MatchWithLineNumber()

Testing MatchRegexpWithLineNumber with test...
output is:
2:This is a test of MatchRegexpWithLineNumber() and MatchWithLineNumber()
4:MatchRegexpWithLineNumber() will also find this TEST occurrence because it's case-insensitive

These behave as `grep -n` does: output the matched line prefixed by the
line number.
@bitfield
Copy link
Owner

Hey @jessp01, thanks for the PR!

This is a neat idea. Could you post a comment with some example code showing how this feature would solve a user's problem? I can't think of any situation in which I've needed to use the equivalent of grep -n recently, but I'm sure there are some.

@jessp01
Copy link
Contributor Author

jessp01 commented Feb 26, 2025

Hi @bitfield ,

Yes, as it so happens, I am working on a Glow feature I need this for right now:)

You can see a screencast where I demonstrate it.

I'm using your module here
The new MatchRegexpWithLineNumber() will enable me to open the relevant file and jump to the line where the first match was found.

Later, I will also use it to introduce search functionality in the built-in glow Pager (when it displays a given file).

PS
Had Go supported optional params or method overloading, I could have revised the original Match methods without breaking the API but since it doesn't, I had to introduce these two new methods.

@bitfield
Copy link
Owner

What I'm really looking for is an example of code that uses MatchRegexpWithLineNumber (for example). Then we can see what it would look like in context and what kind of problems it solves.

@jessp01
Copy link
Contributor Author

jessp01 commented Feb 26, 2025

Well, as I wrote, I'd replace the call to MatchRegexp() here: https://github.com/jessp01/glow/blob/README/ui/stash.go#L894 with MatchRegexpWithLineNumber(), split the output by ":" so I have the line number in which the string appears and the actual string and use the former to open the file and move to the line number in question.

And actually, as I write this, I think it would make sense to go a step further and revise these new methods so that they return the string and line number as separate values (in addition to outputting with fmt.Fprintln). If you agree, I'll make the change.

@bitfield
Copy link
Owner

Sure, try a few different ideas and see what looks best and makes most sense to you. Post the most compelling example here as a comment. If it's a problem lots of people have, and you come up with an elegant solution that justifies the cost of increasing the script API surface, I'll certainly consider it.

@jessp01
Copy link
Contributor Author

jessp01 commented Feb 26, 2025

Hi @bitfield ,

Post the most compelling example here as a comment.

I already have done:) This is a real feature I am working on that made this functionality necessary. Did you look at the code, pull description and screencast I've referenced?

@bitfield
Copy link
Owner

I think I'm missing something somewhere—the code you linked seems to use MatchRegexp, as it currently is, but not MatchRegexpWithLineNumber, as you propose. Do you have an example of using MatchRegexpWithLineNumber? That's what I'm interested in seeing.

@bitfield
Copy link
Owner

bitfield commented Mar 5, 2025

I'll close this for now, since it seems to have stalled—if you'd like to pick it up again at any point in the future, please do.

@bitfield bitfield closed this Mar 5, 2025
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.

2 participants