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

Use mypy annotations #9

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
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
26 changes: 15 additions & 11 deletions colcon_cargo/package_identification/cargo.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
# Copyright 2018 Easymov Robotics
# Licensed under the Apache License, Version 2.0

from collections.abc import MutableMapping
from typing import Optional, List
from pathlib import Path

from colcon_core.package_identification \
import PackageIdentificationExtensionPoint
from colcon_core.plugin_system import satisfies_version
Expand Down Expand Up @@ -36,12 +40,11 @@ def identify(self, metadata): # noqa: D102
metadata.dependencies['run'] |= data['depends']


def extract_data(cargo_toml):
def extract_data(cargo_toml: Path) -> Optional[dict]:
"""
Extract the project name and dependencies from a Cargo.toml file.

:param Path corgo_toml: The path of the Cargo.toml file
:rtype: dict
:param corgo_toml: The path of the Cargo.toml file
bergercookie marked this conversation as resolved.
Show resolved Hide resolved
"""
content = {}

Expand All @@ -63,26 +66,27 @@ def extract_data(cargo_toml):
return data


def extract_project_name(content):
def extract_project_name(content: MutableMapping) -> Optional[str]:
"""
Extract the Cargo project name from the Cargo.toml file.

:param str content: The Cargo.toml parsed dictionnary
:param content: The Cargo.toml parsed dictionnary
:returns: The project name, otherwise None
:rtype: str
"""
try:
return content['package']['name']
except KeyError:
return None


def extract_dependencies(content):
def extract_dependencies(content: MutableMapping) -> List[str]:
"""
Extract the dependencies from the Cargo.toml file.

:param str content: The Cargo.toml parsed dictionnary
:returns: The dependencies name
:rtype: list
:param content: The Cargo.toml parsed dictionnary
:returns: Packages listed in the dependencies section
"""
return list(content['dependencies'].keys())
try:
list(content['dependencies'].keys())
except KeyError:
return []
24 changes: 15 additions & 9 deletions colcon_cargo/task/cargo/__init__.py
Original file line number Diff line number Diff line change
@@ -1,30 +1,36 @@
# Copyright 2018 Easymov Robotics
# Licensed under the Apache License, Version 2.0

from typing import Optional

import os
import shutil

from colcon_core.environment_variable import EnvironmentVariable

"""Environment variable to override the Cargo executable"""
# Environment variable to override the Cargo executable
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd keep this as docstring

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Depends what you use to generate the documentation, but if we're talking about sphinx, it doesn't support extracting docstrings from variables unfortunately

CARGO_COMMAND_ENVIRONMENT_VARIABLE = EnvironmentVariable(
'CARGO_COMMAND', 'The full path to the Cargo executable')


def which_executable(environment_variable, executable_name):
def which_executable(environment_variable: str, executable_name: str) \
-> Optional[str]:
"""
Determine the path of an executable.

An environment variable can be used to override the location instead of
relying on searching the PATH.
:param str environment_variable: The name of the environment variable
:param str executable_name: The name of the executable
:rtype: str

:param environment_variable: The name of the environment variable
:param executable_name: The name of the executable, or None if that's not
found
"""
value = os.getenv(environment_variable)
if value:
return value
return shutil.which(executable_name)
if environment_variable in os.environ:
Copy link
Contributor

Choose a reason for hiding this comment

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

This change is equivalent to the existing code, why the change?

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 find queyring directly the os.environ more readable in a couple of ways:

  • There's no need for the intermediate value variable, which doesn't say much about the value it holds anyway
  • the environment in python is just a dictionary, so using an os function, getenv , to manipulate it sounds unnecessary since there are already established ways of querying a dictionary.

In any case, it's not terribly important, I can revert the change if you think it was better before.

return os.environ[environment_variable]

which = shutil.which(executable_name)
if which:
Copy link
Contributor

Choose a reason for hiding this comment

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

This already returns None if the executable_name can't be found, so it can be simplified as:

return shutil.which(executable_name)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, agree, I 'll change it back

return which


CARGO_EXECUTABLE = which_executable(
Expand Down
1 change: 1 addition & 0 deletions setup.cfg
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ install_requires =
# to set an environment variable when a package installs a library
colcon-library-path
toml
typing
packages = find:
tests_require =
flake8
Expand Down