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

Add type checking to several `util` files #7998

Merged
merged 5 commits into from Jul 3, 2019

Conversation

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

commented Jul 2, 2019

The util code—excluding the metaprogramming files—is a good place to start adding type hints to Pants because it is composed of simple pure functions and is used widely throughout the codebase.

This adds some initial hints to act as a demo for type hints in Pants and get us slightly closer to critical mass.

@Eric-Arellano

This comment has been minimized.

Copy link
Contributor Author

commented Jul 2, 2019

This PR was queued up from a few weeks ago so I figured I would put it up, even though it's not as comprehensive as I would have liked. We can finish adding hints to util in later followup PRs—for now, I'll be focusing on remoting though.

@blorente
Copy link
Contributor

left a comment

Thanks!

@@ -35,7 +37,7 @@ def _cmd_exists(cmd):
}


def ui_open(*files):
def ui_open(*files: str) -> None:

This comment has been minimized.

Copy link
@blorente

blorente Jul 3, 2019

Contributor

Huh, this is a strange way of representing this. I understand that the type annotation means "zero or more arguments, all of which are strings", but in the code the name files will be bound to a List[str], not a str.

Not saying this is wrong, just a small corner of python typing I find odd :)

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Jul 3, 2019

Author Contributor

I think technically files is a Sequence[str], which is the metaclass(?) for List.

Agreed, though, that is strange. Kwargs is the same:

def my_function(**kwargs: Union[str, int]):
 ...
@@ -18,7 +20,7 @@
Pid = int


def get_os_name(uname_result=None):
def get_os_name(uname_result: Optional[posix.uname_result] = None) -> str:

This comment has been minimized.

Copy link
@blorente

blorente Jul 3, 2019

Contributor

Things like posix.uname_result are what I want types for :)

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Jul 3, 2019

Author Contributor

Yes! So much clearer. Btw, I found this via the typeshed repository, the entry for os.uname(). They have fantastic types for the stdlib (and some 3rd parties).

--

Another trick: if you use IntelliJ or Pycharm, hold down the command key and hover over any call to get_os_name :)

@benjyw

benjyw approved these changes Jul 3, 2019

@Eric-Arellano Eric-Arellano merged commit 8268e27 into pantsbuild:master Jul 3, 2019

1 check passed

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

@Eric-Arellano Eric-Arellano deleted the Eric-Arellano:type-check-util branch Jul 3, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.