Create a function for pattern-matching and error checking?

[FiloSottile]

We have two common base patterns:

mobj = re.search(REGEX, TEXT)
if mobj is None:
    raise ExtractorError(u'Unable to extract %s' % THING)
video_title = mobj.group(1)

mobj = re.search(REGEX, TEXT)
if mobj is None:
    self._downloader.report_warning(u'unable to extract %s' % THING)
    video_description = None
else:
    video_description = mobj.group(1)

So I was thinking about a helper function like

def search_regex(pattern, string, name, fatal=True, flags=0):
    mobj = re.search(pattern, string, flags)
    if mobj is None and fatal:
        raise ExtractorError(u'Unable to extract %s' % name)
    elif mobj is None:
        self._downloader.report_warning(u'unable to extract %s' % name)
        return None
    else:
        return mobj.group(1) # Or maybe the first matched group,
        # to accomodate cases like (?:"([^"]+)"|\'([^\']+)\')

If you like it I can put up a PR.

[FiloSottile]

(pinging devs @phihag @jaimeMF)

[phihag]

Go ahead and start changing, this looks fine to me. Although I'd really prefer HTML parsing + xpaths for a lot of these checks.

[FiloSottile]

Eh, me too, I'm a huge BeautifulSoup fan, but I fear that without
introducing dependencies there are not elegant and reliable HTML (not
simply XML) 2.6 - 3.3 parsing solutions...

Filippo Valsorda

2013/5/20 Philipp Hagemeister notifications@github.com

Go ahead and start changing, this looks fine to me. Although I'd really
prefer HTML parsing + xpaths for a lot of these checks.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/847#issuecomment-18136112
.

[jaimeMF]

I would personally return the mobj, since you can pass a pattern with multiple groups.

[FiloSottile]

Umh... there would not be much gain, then, as you have to check if the mobj
is Null...

My idea is that of supporting only a single return value, and to take any
matched group (see the comments to the code above)

Filippo Valsorda

2013/5/20 Jaime Marquínez Ferrándiz notifications@github.com

I would personally return the mobj, since you can pass a pattern with
multiple groups.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/847#issuecomment-18144591
.

[jaimeMF]

You're right, I was thinking just in fatal errors.

[FiloSottile]

Speaking about errors, I and @jaimeMF thought that we should discuss which extraction failures should be fatal, and which should issue warnings.

My take is:

• title, id, url, and extension (that is, the required info_dict fields) yield fatals
• all the other only warnings, but with a message along the lines of "please report this issue"

[FiloSottile]

So this would be the function

def search_regex(pattern, string, name, fatal=True, flags=0):
    mobj = re.search(pattern, string, flags)
    if mobj is None and fatal:
        raise ExtractorError(u'Unable to extract %s; '
            u'please report this issue on GitHub.' % name)
    elif mobj is None:
        self._downloader.report_warning(u'unable to extract %s; '
            u'please report this issue on GitHub.' % name)
        return None
    else:
        # return the first matched group
        return next(g for g in mobj.groups() if g is not None)

[jaimeMF]

Just one note, some IEs assign '' to fields that can't be found, we should choose to use it or None, or maybe pass an optional default_value to search_regex

[phihag]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

On 05/29/2013 08:10 AM, Jaime Marquínez Ferrándiz wrote:

Just one note, some IEs assign '' to fields that can't be found
(...)
Those IEs should be fixed. If you can't find a field, leave it out. If
you can find it, but it's unset, set it to None.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.12 (GNU/Linux)

iEYEAREKAAYFAlGlrikACgkQ9eq1gvr7CFy+eQCbBSWFO78LrhVa2UL0DFydIHL/
2I4AniNbJiwik8dRb7zJr4HD4Z99KBHA
=PZ+T
-----END PGP SIGNATURE-----

[FiloSottile]

Ok, great, Anna is in the process of deploying the function, I'll have her
enforce this.

However, I feel like that in output filenames we should represent unset or
None as 'nd' and/or 'unknown', or something like that.

While we are at it, @phihag have you expressed your preference on which
fields missing should raise a fatal and which a warning? I would go for id,
url and title fatal.

On Wednesday, May 29, 2013, Philipp Hagemeister wrote:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

On 05/29/2013 08:10 AM, Jaime Marquínez Ferrándiz wrote:

Just one note, some IEs assign '' to fields that can't be found
(...)
Those IEs should be fixed. If you can't find a field, leave it out. If
you can find it, but it's unset, set it to None.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.12 (GNU/Linux)

iEYEAREKAAYFAlGlrikACgkQ9eq1gvr7CFy+eQCbBSWFO78LrhVa2UL0DFydIHL/
2I4AniNbJiwik8dRb7zJr4HD4Z99KBHA
=PZ+T
-----END PGP SIGNATURE-----

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/847#issuecomment-18600170
.

Filippo Valsorda

[FiloSottile]

My latest iteration of the function, including a default value and support for multiple fallback patterns:

def search_regex(pattern, string, name, default=None, fatal=True, flags=0):
    """
    Perform a regex search on the given string, using a single or a list of
    patterns returning the first matching group.
    In case of failure return a default value or raise a WARNING or a
    ExtractorError, depending on fatal, specifying the field name.
    """
    if type(pattern) in (type(''), type(u''), re.RegexObject):
        mobj = re.search(pattern, string, flags)
    else:
        for p in pattern:
            mobj = re.search(p, string, flags)
            if mobj: break

    if sys.stderr.isatty() and os.name != 'nt':
        _name = u'\033[0;34m%s\033[0m' % name
    else:
        _name = name

    if mobj:
        # return the first matching group
        return next(g for g in mobj.groups() if g is not None)
    elif default is not None:
        return default
    elif fatal:
        raise ExtractorError(u'Unable to extract %s; '
            u'please report this issue on GitHub.' % _name)
    else:
        self._downloader.report_warning(u'unable to extract %s; '
            u'please report this issue on GitHub.' % _name)
        return None