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

Remove str() calls in call/shell functions using destructuring syntax #387

Merged
merged 2 commits into from
Jun 25, 2020

Conversation

joerick
Copy link
Contributor

@joerick joerick commented Jun 23, 2020

While working on #386 I came across a way to remove those str() calls when combining Path and str objects. The trick is to use array destructuring syntax, e.g. ['pip', 'wheel', path_object, *flags], rather than +. This way the typechecker assumes you intended to mix the types and gives you the type List[Union[str, PathLike]].

Copy link
Member

@YannickJadoul YannickJadoul left a comment

Choose a reason for hiding this comment

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

Uuuuh, alright, that looks nice! Great discovery :-)
Just noting: if we need to use it multiple times, adding a statement like CallArgs = Sequence[Union[str, PathLike]] should work to create a shorter alias (not sure if it's worth it in this case, though).

Two minor style things:

  • I had tried to keep the typing imports separate from the other imports, since they do not really import any functionality (and I'm still slightly annoyed we have to import those ;-) ).
  • The exploded call(['pip', 'wheel', ...]) lines are spread over many lines. Is there any way to make this more compact? E.g., have something like
call(['pip', 'wheel',
        options.package_dir.resolve(),
        '-w', built_wheel_dir,
        '--no-deps',
        *get_build_verbosity_extra_flags(options.build_verbosity)
       ], env=env)

or so? But maybe I like long lines of code too much :-D

@joerick
Copy link
Contributor Author

joerick commented Jun 25, 2020

  • I had tried to keep the typing imports separate from the other imports, since they do not really import any functionality (and I'm still slightly annoyed we have to import those ;-) ).

These changes were by VScode's 'sort imports' command. I'd guess isort would do something similar. Happy to change it back if you like? But I think we just embrace it - Python types are kinda weird, but we're in for the ride! :) also, from os import PathLike - this is just a type - should that be separate? gets kinda fuzzy.

  • The exploded call(['pip', 'wheel', ...]) lines are spread over many lines. Is there any way to make this more compact? E.g., have something like

In retrospect, I agree. I've reduced it down.

@YannickJadoul
Copy link
Member

These changes were by VScode's 'sort imports' command. I'd guess isort would do something similar. Happy to change it back if you like? But I think we just embrace it - Python types are kinda weird, but we're in for the ride! :) also, from os import PathLike - this is just a type - should that be separate? gets kinda fuzzy.

Yeah, I don't really mind. Just noticed. And since no one made remarks when I added them like that, I had assumed it was appreciated. But if it goes against code formatters, I don't feel that strongly ;-)

@joerick joerick merged commit db9d658 into master Jun 25, 2020
@joerick joerick deleted the call-types branch August 29, 2020 20:44
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.

2 participants