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

Use set literals & set comprehensions where possible #6376

Merged
merged 6 commits into from Aug 22, 2018

Conversation

Projects
None yet
3 participants
@Eric-Arellano
Copy link
Contributor

Eric-Arellano commented Aug 21, 2018

Description

Uses set literals, i.e. {foo, bar} and set comprehensions, i.e. {foo(x) for x in bar} wherever possible, instead of the constructor set([foo, bar]).

Motivation

The set literal syntax is preferred for both readability and performance.

According to https://renzo.lucioni.xyz/pythons-set-literals/, set literals average an execution twice as fast as the set constructor, in part because set([foo, bar]) generates the intermediate list before converting it to a set.

How change was generated

Used this script to generate proposed fixes, which then prompts a review of each proposal and requires actively confirming by pressing y.

Regex used for normal set:

def _generate_set_fix(line: str) -> str:
  list_regex = r'set\(\[(?P<literal>.*)\]\)'
  tuple_regex = r'set\(\((?P<literal>.*)\)\)'
  list_fix = re.sub(list_regex, '{\g<literal>}', line)
  tuple_fix = re.sub(tuple_regex, '{\g<literal>}', list_fix)
  return tuple_fix

Regex used for frozenset:

def _generate_frozenset_fix(line: str) -> str:
  list_regex = r'\(\[(?P<literal>.*)\]\)'
  tuple_regex = r'\(\((?P<literal>.*)\)\)'
  list_fix = re.sub(list_regex, '({\g<literal>})', line)
  tuple_fix = re.sub(tuple_regex, '({\g<literal>})', list_fix)
  return tuple_fix

Regex used for comprehensions:

def _generate_comprehension_fix(line: str) -> str:
  regex = r'set\((?P<comp>.*)\)'
  return re.sub(regex, '{\g<comp>}', line)
@stuhood
Copy link
Member

stuhood left a comment

Thanks!

@baroquebobcat
Copy link
Contributor

baroquebobcat left a comment

Looks good!

I did a bit of grepping myself and saw that there are also instances of using tuples instead of lists (set((1,2))) and ~10 comprehensions that could be made into literals (set(x for x in blah))

@Eric-Arellano

This comment has been minimized.

Copy link
Contributor

Eric-Arellano commented Aug 21, 2018

Good point Nora! Let’s wait to merge until I update that.

@baroquebobcat

This comment has been minimized.

Copy link
Contributor

baroquebobcat commented Aug 21, 2018

Sounds good!

@@ -38,7 +38,7 @@ def set_mapper(self, fast=False):
raise NotImplementedError

def owner(self, owner, f):
self.assertEqual(set(owner), set(i.spec for i in self.get_mapper().target_addresses_for_source(f)))
self.assertEqual({owner}, {i.spec for i in self.get_mapper().target_addresses_for_source(f)})

This comment has been minimized.

@baroquebobcat

baroquebobcat Aug 21, 2018

Contributor

set(owner) -> {owner} might not be right. I think it'll mean that owner will be an element of the set, rather than copied into one.

@@ -74,7 +74,7 @@ def assert_digest(self, filespecs_or_globs, expected_files):
scheduler = self.mk_scheduler(rules=create_fs_rules(), project_tree=project_tree)
result = self.execute(scheduler, Snapshot, self.specs(filespecs_or_globs))[0]
# Confirm all expected files were digested.
self.assertEqual(set(expected_files), set(f.path for f in result.files))
self.assertEqual({expected_files}, {f.path for f in result.files})

This comment has been minimized.

@baroquebobcat

baroquebobcat Aug 21, 2018

Contributor

set(expected_files) -> {expected_files} might not be right. I think it'll mean that expected_files will be an element of the set, rather than copied into one.

This comment has been minimized.

@Eric-Arellano

Eric-Arellano Aug 22, 2018

Contributor

You're totally right. Thanks for the close review!

Eric-Arellano added some commits Aug 22, 2018

@stuhood stuhood merged commit d24cfe2 into pantsbuild:master Aug 22, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Eric-Arellano Eric-Arellano deleted the Eric-Arellano:string-literals branch Nov 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment