Skip to content

Some code simplifications with the return statement #877

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions vdirsyncer/cli/config.py
Original file line number Diff line number Diff line change
@@ -242,7 +242,7 @@ def __init__(self, full_config, name, options):
def _process_conflict_resolution_param(self, conflict_resolution):
if conflict_resolution in (None, 'a wins', 'b wins'):
return conflict_resolution
elif isinstance(conflict_resolution, list) and \
if isinstance(conflict_resolution, list) and \
len(conflict_resolution) > 1 and \
conflict_resolution[0] == 'command':
def resolve(a, b):
@@ -257,8 +257,7 @@ def inner():
return ui_worker.put(inner)

return resolve
else:
raise ValueError('Invalid value for `conflict_resolution`.')
raise ValueError('Invalid value for `conflict_resolution`.')

# The following parameters are lazily evaluated because evaluating
# self.config_a would expand all `x.fetch` parameters. This is costly and
9 changes: 4 additions & 5 deletions vdirsyncer/cli/discover.py
Original file line number Diff line number Diff line change
@@ -50,14 +50,13 @@ def collections_for_pair(status_path, pair, from_cache=True,
return list(_expand_collections_cache(
rv['collections'], pair.config_a, pair.config_b
))
elif rv:
if rv:
raise exceptions.UserError('Detected change in config file, '
'please run `vdirsyncer discover {}`.'
.format(pair.name))
else:
raise exceptions.UserError('Please run `vdirsyncer discover {}` '
' before synchronization.'
.format(pair.name))
raise exceptions.UserError('Please run `vdirsyncer discover {}` '
' before synchronization.'
.format(pair.name))

logger.info('Discovering collections for pair {}' .format(pair.name))

5 changes: 2 additions & 3 deletions vdirsyncer/cli/utils.py
Original file line number Diff line number Diff line change
@@ -181,7 +181,7 @@ def get_status_path(base_path, pair, collection=None, data_type=None):
def load_status(base_path, pair, collection=None, data_type=None):
path = get_status_path(base_path, pair, collection, data_type)
if not os.path.exists(path):
return None
return
Copy link
Member

Choose a reason for hiding this comment

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

👍

assert_permissions(path, STATUS_PERMISSIONS)

with open(path) as f:
@@ -268,8 +268,7 @@ def storage_instance_from_config(config, create=True):
config = handle_collection_not_found(
config, config.get('collection', None), e=str(e))
return storage_instance_from_config(config, create=False)
else:
raise
raise
except Exception:
return handle_storage_init_error(cls, new_config)

6 changes: 2 additions & 4 deletions vdirsyncer/http.py
Original file line number Diff line number Diff line change
@@ -37,10 +37,10 @@ def prepare_auth(auth, username, password):
if username and password:
if auth == 'basic' or auth is None:
return (username, password)
elif auth == 'digest':
if auth == 'digest':
from requests.auth import HTTPDigestAuth
return HTTPDigestAuth(username, password)
elif auth == 'guess':
if auth == 'guess':
Copy link
Member

Choose a reason for hiding this comment

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

I don't really like this change.

It makes it far harder to figure out the flow. At a glance, I'd get the impression that this code run if auth == 'guess', but there's actually a return nested further above.

The elif makes it a bit more obvious that these are mutually exclusive code-paths.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In all programming languages, writing else after return has no added value.

Copy link
Member

Choose a reason for hiding this comment

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

It helps readability, IMO.

try:
from requests_toolbelt.auth.guess import GuessAuth
except ImportError:
@@ -57,8 +57,6 @@ def prepare_auth(auth, username, password):
elif auth:
raise exceptions.UserError('You need to specify username and password '
'for {} authentication.'.format(auth))
else:
return None


def prepare_verify(verify, verify_fingerprint):
10 changes: 4 additions & 6 deletions vdirsyncer/storage/dav.py
Original file line number Diff line number Diff line change
@@ -122,7 +122,7 @@ def _parse_xml(content):

def _merge_xml(items):
if not items:
return None
return
rv = items[0]
for item in items[1:]:
rv.extend(item.iter())
@@ -138,9 +138,7 @@ def _fuzzy_matches_mimetype(strict, weak):
return True

mediatype, subtype = strict.split('/')
if subtype in weak:
return True
return False
return subtype in weak
Copy link
Member

Choose a reason for hiding this comment

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

This is a much welcome cleanup though.



class Discover:
@@ -236,7 +234,7 @@ def _check_collection_resource_type(self, response):
props = _merge_xml(response.findall(
'{DAV:}propstat/{DAV:}prop'
))
if props is None or not len(props):
if props is None or not props:
dav_logger.debug('Skipping, missing <prop>: %s', response)
return False
if props.find('{DAV:}resourcetype/' + self._resourcetype) \
@@ -587,7 +585,7 @@ def _parse_prop_responses(self, root, handled_hrefs=None):
continue

props = response.findall('{DAV:}propstat/{DAV:}prop')
if props is None or not len(props):
if props is None or not props:
dav_logger.debug('Skipping {!r}, properties are missing.'
.format(href))
continue
4 changes: 2 additions & 2 deletions vdirsyncer/sync/status.py
Original file line number Diff line number Diff line change
@@ -246,12 +246,12 @@ def _get_impl(self, ident, side, table):
.format(side=side, table=table),
(ident,)).fetchone()
if res is None:
return None
return

if res['hash'] is None: # FIXME: Implement as constraint in db
assert res['href'] is None
assert res['etag'] is None
return None
return

res = dict(res)
return ItemMetadata(**res)
6 changes: 2 additions & 4 deletions vdirsyncer/utils.py
Original file line number Diff line number Diff line change
@@ -21,8 +21,7 @@

def expand_path(p):
p = os.path.expanduser(p)
p = os.path.normpath(p)
return p
return os.path.normpath(p)


def split_dict(d, f):
@@ -181,8 +180,7 @@ def generate_href(ident=None, safe=SAFE_UID_CHARS):
'''
if not ident or not href_safe(ident, safe):
return str(uuid.uuid4())
else:
return ident
return ident


def synchronized(lock=None):
18 changes: 7 additions & 11 deletions vdirsyncer/vobject.py
Original file line number Diff line number Diff line change
@@ -233,8 +233,7 @@ def _get_item_type(components, wrappers):

if not i:
return None, None
else:
raise ValueError('Not sure how to join components.')
raise ValueError('Not sure how to join components.')


class _Component:
@@ -294,11 +293,10 @@ def parse(cls, lines, multiple=False):

if multiple:
return rv
elif len(rv) != 1:
if len(rv) != 1:
raise ValueError('Found {} components, expected one.'
.format(len(rv)))
else:
return rv[0]
return rv[0]

def dump_lines(self):
yield f'BEGIN:{self.name}'
@@ -315,8 +313,7 @@ def __delitem__(self, key):
for line in lineiter:
if line.startswith(prefix):
break
else:
new_lines.append(line)
new_lines.append(line)
else:
break

@@ -338,10 +335,9 @@ def __contains__(self, obj):
if isinstance(obj, type(self)):
return obj not in self.subcomponents and \
not any(obj in x for x in self.subcomponents)
elif isinstance(obj, str):
if isinstance(obj, str):
return self.get(obj, None) is not None
else:
raise ValueError(obj)
raise ValueError(obj)

def __getitem__(self, key):
prefix_without_params = f'{key}:'
@@ -351,7 +347,7 @@ def __getitem__(self, key):
if line.startswith(prefix_without_params):
rv = line[len(prefix_without_params):]
break
elif line.startswith(prefix_with_params):
if line.startswith(prefix_with_params):
rv = line[len(prefix_with_params):].split(':', 1)[-1]
break
else: