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

make match() on Enum into a top-level function in meta.py #8504

Merged
merged 9 commits into from Dec 25, 2019

Conversation

cosmicexplorer
Copy link
Contributor

Problem

Importing via from pants.util.collections import Enum induces an indirection which isn't necessary for most users of Enum (the ones that don't use the .match() function).

Solution

  • Use the normal stdlib enum type.
  • Make match into a top-level function in meta.py.
  • Migrate the codebase.

Result

An awkward import is resolved, and a new import is born! Complete credit for the solution goes to @jsirois in the #engine channel on our Slack.

Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

Awesome! Thank you Danny and John!

src/python/pants/util/meta.py Outdated Show resolved Hide resolved
@@ -18,7 +20,7 @@ def current(cls) -> 'Platform':

@memoized_property
def runtime_lib_path_env_var(self):
return self.match({
return match(self, {
Copy link
Member

@jsirois jsirois Oct 19, 2019

Choose a reason for hiding this comment

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

I guess, but it seems much more straight forward to say:

class Platform(Enum):
  darwin = ("darwin", "DYLD_LIBRARY_PATH")
  linux = ("linux", "LD_LIBRARY_PATH")

  def __new__(cls, value, runtime_lib_path_env_var) -> 'Platform':
    instance = super().__new__(cls, value)
    instance._runtime_lib_path_env_var = runtime_lib_path_env_var
    return instance

  @property
  def runtime_lib_path_env_var(self):
    return self._runtime_lib_path_env_var

Or even:

@dataclass(frozen=True)
class PlatformInfo:
  name: str
  runtime_lib_path_env_var: str


class Platform(Enum):
  darwin = PlatformInfo("darwin", "DYLD_LIBRARY_PATH")
  linux = PlatformInfo("linux", "LD_LIBRARY_PATH")
  
  # Optional for less code ripple, but arguably you'd just kill and have users say:
  # `platform_instance.value.runtime_lib_path_env_var` themselves.
  @property
  def runtime_lib_path_env_var(self):
    return self.value.runtime_lib_path_env_var

IOW if the enum value passed to match is self, that seems like a smell that the enum value should be a struct instead of a single value. If you agree I'd definitely follow up and not scope creep this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree and will move to a followup PR!

src/python/pants/util/meta.py Outdated Show resolved Hide resolved
src/python/pants/util/meta.py Outdated Show resolved Hide resolved
Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

Yay! Thanks!

@cosmicexplorer cosmicexplorer force-pushed the make-match-a-method branch 3 times, most recently from f47baec to 28f0f9b Compare October 21, 2019 16:54
@Eric-Arellano
Copy link
Contributor

Hello, was this previously blocked by the broken master CI? Would love to see this land :)

@cosmicexplorer cosmicexplorer force-pushed the make-match-a-method branch 2 times, most recently from 2441636 to 4e99376 Compare November 25, 2019 03:54
@stuhood stuhood removed their request for review November 25, 2019 18:56
Copy link
Contributor

@blorente blorente left a comment

Choose a reason for hiding this comment

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

Very neat solution, thanks!

src/python/pants/engine/platform.py Outdated Show resolved Hide resolved
build-support/bin/check_banned_imports.py Show resolved Hide resolved
Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

Yay! Thank you for finishing this out.

Nit: consider from pants.util.enums import match and using match directly. This fits with the codebase style better.

tests/python/pants_test/util/test_meta.py Outdated Show resolved Hide resolved
Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

Awesome! Thanks Danny!

@cosmicexplorer cosmicexplorer force-pushed the make-match-a-method branch 2 times, most recently from 71ee8d4 to 544d260 Compare December 25, 2019 02:05
@cosmicexplorer cosmicexplorer merged commit fd469cb into pantsbuild:master Dec 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants