The Wayback Machine - https://web.archive.org/web/20200823065744/https://github.com/Flexget/Flexget/pull/2338
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

Update manipulate.py re.search #2338

Open
wants to merge 7 commits into
base: develop
from
Open

Conversation

@chaosmaker
Copy link

chaosmaker commented Feb 12, 2019

re.search only allows for the first hit to be returned. By adding the option to use re.findall multiple matches can be returned to the user using the same method as re.search.

Motivation for changes:

re.search only allows a single hit to be returned during extract

Detailed changes:

  • add boolean to select findall, and return matches identifi

Addressed issues:

  • Fixes # .

Implemented feature requests:

  • Feathub #71.71

Config usage if relevant (new plugin or updated schema):

paste_config_here
added boolean for findall: true

Log and/or tests output (preferably both):

paste output here

To Do:

  • Stuff..
re.search only allows for the first hit to be returned. By adding the option to use re.findall multiple matches can be returned to the user using the same method as re.search.
Copy link
Member

cvium left a comment

A few minor issues that I suggested fixes for. Otherwise it looks fine.

@gazpachoking Any thoughts on this?

flexget/plugins/modify/manipulate.py Outdated Show resolved Hide resolved
flexget/plugins/modify/manipulate.py Outdated Show resolved Hide resolved
flexget/plugins/modify/manipulate.py Outdated Show resolved Hide resolved
flexget/plugins/modify/manipulate.py Outdated Show resolved Hide resolved
@chaosmaker
Copy link
Author

chaosmaker commented Feb 13, 2019

I've altered all instances in manipulate.py with the minor corrections that @cvium provided.

@gazpachoking
Copy link
Member

gazpachoking commented Feb 13, 2019

I'm good with this, should we put an underscore in the option though? find_all?

@gazpachoking
Copy link
Member

gazpachoking commented Feb 13, 2019

Also, it looks like the behavior of only capturing groups is different depending on whether findall is specified. Shouldn't that stay consistent?

@gazpachoking
Copy link
Member

gazpachoking commented Feb 13, 2019

In fact, it looks like this new config,

findall: yes
extract: anything

would be equivalent to this already possible config:

extract: "(?:(anything).*)+"

Is that true? I agree the syntax on that already possible one is more complicated, just trying to figure out the best/simplest way to allow this feature.

@chaosmaker
Copy link
Author

chaosmaker commented Feb 13, 2019

@gazpachoking
Copy link
Member

gazpachoking commented Feb 13, 2019

Aha, seems like python's re module doesn't support repeated capture groups. Well, I'm on board for this then, my 2 questions still remain:

  1. Should the behavior stay consistent with regular extract, and only extract capture groups?
  2. Should we call it find_all rather than findall?

(I'm leaning towards 'yes' on both of these questions.)

@chaosmaker
Copy link
Author

chaosmaker commented Feb 15, 2019

Aha, seems like python's re module doesn't support repeated capture groups. Well, I'm on board for this then, my 2 questions still remain:

  1. Should the behavior stay consistent with regular extract, and only extract capture groups?
  2. Should we call it find_all rather than findall?

(I'm leaning towards 'yes' on both of these questions.)

I'm not 100% sure what you mean by 1. In my understanding findall should only extract capture groups, but if they occur more than once they will all be captured.
I have no issue naming it find_all if you think that this would make it easier to understand what it does.
Once pulled the documentation needs to be changed online too.

chaosmaker added 3 commits Sep 10, 2019
Just altered findall to find_all
@chaosmaker chaosmaker requested a review from cvium Jul 3, 2020
@chaosmaker
Copy link
Author

chaosmaker commented Jul 3, 2020

I didn't realise this was never finished. Can we finalise this pull please?

Copy link
Author

chaosmaker left a comment

Files have been updated to latest development version

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.