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

find: use slash or pipe in sed-like command #1993

Merged
merged 1 commit into from
Jan 31, 2021

Conversation

Exirel
Copy link
Contributor

@Exirel Exirel commented Nov 25, 2020

Description

Attempted fix for #1188

So it was kind of messy because working with backreference to negate them is (not) surprisingly hard to do. I got some help from StackOverflow but I had to let go of the "you can escape the separator to use it inside the substitution/editing text ".

But heh, on the other hand, as one of the authors of the previous regex, if you want both / and | as separator AND having both in a substitution message:

you're shit out of luck because this is the fucking regex of death as it is.

PS: it was on @dgw 's task list, but I did it anyway out of curiosity for regex backreference.

Checklist

  • I have read CONTRIBUTING.md
  • I can and do license this contribution under the EFLv2
  • No issues are reported by make qa (runs make quality and make test)
  • I have tested the functionality of the things this change touches

@Exirel Exirel added this to the 7.1.0 milestone Nov 25, 2020
@Exirel Exirel requested a review from dgw November 25, 2020 11:18
Copy link
Member

@dgw dgw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I unlinked #1188 because that requests support for using any character as delimiter; this PR adds only one alternative, and removes the escaping feature.

This might get skipped for 7.1, not sure. I'd like to experiment with lookarounds for handling escapes, and/or libraries like sedparse that could be used after this plugin gets extracted to its own package in the run-up to Sopel 8 (since we're conservative about dependencies primarily so no one has to install the whole universe even if they disable the whole core set).

sopel/modules/find.py Show resolved Hide resolved
sopel/modules/find.py Outdated Show resolved Hide resolved
sopel/modules/find.py Outdated Show resolved Hide resolved
sopel/modules/find.py Outdated Show resolved Hide resolved
@Exirel
Copy link
Contributor Author

Exirel commented Dec 26, 2020

This might get skipped for 7.1, not sure. I'd like to experiment with lookarounds for handling escapes

Works for me, either way I gave it a shot!

@dgw
Copy link
Member

dgw commented Dec 26, 2020

I'll hold on an approving review for now, but everything is addressed. I just have to do that lookaround research/experiment(s) and/or decide if this is worth breaking the escaped-delimiter feature in a point release.

@Exirel Exirel force-pushed the find-separator-slash-or-pipe branch from d41de94 to 40df6fe Compare December 29, 2020 15:59
@dgw
Copy link
Member

dgw commented Jan 7, 2021

@Exirel Were you thinking of simplifying this to the easier solution of two rules? Though I still want to investigate lookarounds.

@Exirel Exirel force-pushed the find-separator-slash-or-pipe branch from 40df6fe to e18260f Compare January 14, 2021 16:17
@Exirel Exirel requested a review from dgw January 14, 2021 16:18
@Exirel
Copy link
Contributor Author

Exirel commented Jan 14, 2021

@dgw updated, see if you like it better. Escaping is back!

Copy link
Member

@dgw dgw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually like this approach a lot. It just looks like there might be a little mistake in one of the rules?

sopel/modules/find.py Outdated Show resolved Hide resolved
sopel/modules/find.py Outdated Show resolved Hide resolved
@Exirel Exirel requested a review from dgw January 26, 2021 22:51
@Exirel
Copy link
Contributor Author

Exirel commented Jan 26, 2021

Should be better now.

Copy link
Member

@dgw dgw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we can see why this was deemed "the fucking regex of death" at one point. Pattern's so long you get lost in it! 😂

sopel/modules/find.py Outdated Show resolved Hide resolved
sopel/modules/find.py Outdated Show resolved Hide resolved
Copy link
Member

@dgw dgw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, this looks ready to squash out the fixups 🚀

Co-authored-by: dgw <dgw@technobabbl.es>
@Exirel Exirel force-pushed the find-separator-slash-or-pipe branch from 5c974c7 to 5690063 Compare January 31, 2021 14:15
@dgw dgw merged commit ac14f1b into sopel-irc:master Jan 31, 2021
@Exirel Exirel deleted the find-separator-slash-or-pipe branch May 1, 2021 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants