Page MenuHomePhabricator

phabricator: Pass through diffoptions
Needs RevisionPublic

Authored by sfink on Aug 28 2020, 5:00 PM.

Details

Reviewers
Alphare
indygreg
Group Reviewers
hg-reviewers

Diff Detail

Repository
rHG Mercurial
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

sfink created this revision.Aug 28 2020, 5:00 PM

Nothing uses this directly, but it makes it much easier to another extension to call into this code and be able to request different diff options

Alphare accepted this revision.Sep 10 2020, 4:17 AM
Alphare added a subscriber: Alphare.
Alphare added inline comments.
hgext/phabricator.py
725

I know it's not technically part of this change, but making 32767 a "constant" with an explanation or at least a good name would be nice.

indygreg requested changes to this revision.Sep 17 2020, 10:09 PM
indygreg added a subscriber: indygreg.
indygreg added inline comments.
hgext/phabricator.py
721

[] and {} or any dynamic value should not be used as an argument default because the initially passed value will be used on subsequent invocations if it isn't passed. See https://docs.python-guide.org/writing/gotchas/#mutable-default-arguments.

For this pattern, do something like:

def func(optional=None):
    optional = optional or {}

or

def func(optional=None):
    if optional is None:
        optional = {}
726

It's weird to pass an options dict/argument only to use a single key.

Perhaps we should do something like this instead:

options = dict(options or {})
options['git'] = True

diffopts = mdiff.diffopts(**options)
1082

Mutable default argument.

This revision now requires changes to proceed.Sep 17 2020, 10:09 PM