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

Manage black version using requirement file #1748

Merged
merged 7 commits into from
Oct 23, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
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
14 changes: 8 additions & 6 deletions .pre-commit-config.yaml
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
# https://pre-commit.com/#repository-local-hooks
repos:
- repo: https://github.com/python/black
rev: 19.3b0
hooks:
- id: black
language_version: python3.7
exclude_types: ['markdown', 'ini', 'toml', 'rst']
- repo: local
hooks:
- id: black
name: black
entry: black
language: system
types: [python]
6 changes: 0 additions & 6 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,6 @@ matrix:

install:
# Install the code requirements
- mkdir $HOME/bin-black
- wget -O $HOME/bin-black/black https://github.com/python/black/releases/download/19.10b0/black
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, good to get rid of this 😩

- chmod +x $HOME/bin-black/black
- export PATH=$PATH:$HOME/bin-black
- black --version

- make init

# Install Docs requirements
Expand Down
40 changes: 34 additions & 6 deletions DEVELOPMENT_GUIDE.rst
Original file line number Diff line number Diff line change
Expand Up @@ -28,16 +28,44 @@ Setup Python locally using `pyenv`_

2. Install Additional Tooling
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1. Black
~~~~~~~~
Black
'''''
We format our code using `Black`_ and verify the source code is black compliant
in Appveyor during PRs. You can find installation instructions on `Black's docs`_.
in Appveyor during PRs. Black will be installed automatically with ``make init``.

After installing, you can check your formatting through our Makefile by running `make black-check`. To automatically update your code to match our formatting, please run `make black`. You can also integrate Black directly in your favorite IDE (instructions
can be found `here`_)
After installing, you can run our formatting through our Makefile by
``make black`` or integrating Black directly in your favorite IDE
(instructions can be found `here <https://black.readthedocs.io/en/stable/editor_integration.html>`__)

(workaround) Integrating Black directly in your favorite IDE
""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""

Since black is installed in virtualenv, when you follow `this
instruction <https://black.readthedocs.io/en/stable/editor_integration.html>`__,
``which black`` might give you this

::

(samtranslator37) $ where black
/Users/<username>/.pyenv/shims/black

However, IDEs such as PyCharm (using FileWatcher) will have a hard time
invoking ``/Users/<username>/.pyenv/shims/black`` and this will happen:

::

pyenv: black: command not found

The `black' command exists in these Python versions:
3.7.2/envs/samtranslator37
samtranslator37

A simple workaround is to use
``/Users/<username>/.pyenv/versions/samtranslator37/bin/black`` instead of
``/Users/<username>/.pyenv/shims/black``.

Pre-commit
~~~~~~~~~~
''''''''''
If you don't wish to manually run black on each pr or install black manually, we have integrated black into git hooks through `pre-commit`_.
After installing pre-commit, run `pre-commit install` in the root of the project. This will install black for you and run the black formatting on
commit.
Expand Down
3 changes: 3 additions & 0 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,9 @@ dev: test
# Verifications to run before sending a pull request
pr: black-check init dev

# Verifications to run before sending a pull request, skipping black check because black requires Python 3.6+
pr2.7: init dev
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: assuming existing code is black-compliant, would be nice if we didn't have to introduce new Python 2-specific cruft, but I'm assuming it doesn't play too well with pip et al. 🤒

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When we run CI, there are multiple env including Python2.7, 3.6, 3.7 and 3.8. The code style will be checked in Python3 .X though. Here we just skip the check in one of the env, I think it should be ok 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI, why the old way (download black binary directly) won't work:

psf/black#1669


define HELP_MESSAGE

Usage: $ make [TARGETS]
Expand Down
3 changes: 3 additions & 0 deletions requirements/dev.txt
Original file line number Diff line number Diff line change
Expand Up @@ -15,3 +15,6 @@ requests>=2.20.0

# CLI requirements
docopt>=0.6.2

# formatter
black==20.8b1; python_version >= '3.6'
3 changes: 1 addition & 2 deletions samtranslator/model/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -355,8 +355,7 @@ def to_cloudformation(self, **kwargs):


class SamResourceMacro(ResourceMacro):
"""ResourceMacro that specifically refers to SAM (AWS::Serverless::*) resources.
"""
"""ResourceMacro that specifically refers to SAM (AWS::Serverless::*) resources."""

# SAM resources can provide a list of properties that they expose. These properties usually resolve to
# CFN resources that this SAM resource generates. This is provided as a map with the following format:
Expand Down
6 changes: 4 additions & 2 deletions samtranslator/model/api/http_api_generator.py
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,8 @@ def _construct_api_domain(self, http_api):
self.domain["EndpointConfiguration"] = "REGIONAL"
elif endpoint not in ["REGIONAL"]:
raise InvalidResourceException(
self.logical_id, "EndpointConfiguration for Custom Domains must be one of {}.".format(["REGIONAL"]),
self.logical_id,
"EndpointConfiguration for Custom Domains must be one of {}.".format(["REGIONAL"]),
)
domain_config["EndpointType"] = endpoint
domain_config["CertificateArn"] = self.domain.get("CertificateArn")
Expand Down Expand Up @@ -352,7 +353,8 @@ def _construct_alias_target(self, domain):
alias_target["DNSName"] = fnGetAtt(self.domain.get("ApiDomainName"), "RegionalDomainName")
else:
raise InvalidResourceException(
self.logical_id, "Only REGIONAL endpoint is supported on HTTP APIs.",
self.logical_id,
"Only REGIONAL endpoint is supported on HTTP APIs.",
)
return alias_target

Expand Down
12 changes: 10 additions & 2 deletions samtranslator/model/iam.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,11 @@ def construct_assume_role_policy_for_service_principal(cls, service_principal):
document = {
"Version": "2012-10-17",
"Statement": [
{"Action": ["sts:AssumeRole"], "Effect": "Allow", "Principal": {"Service": [service_principal]},}
{
"Action": ["sts:AssumeRole"],
"Effect": "Allow",
"Principal": {"Service": [service_principal]},
}
],
}
return document
Expand All @@ -43,7 +47,11 @@ def stepfunctions_assume_role_policy(cls):
document = {
"Version": "2012-10-17",
"Statement": [
{"Action": ["sts:AssumeRole"], "Effect": "Allow", "Principal": {"Service": ["states.amazonaws.com"]},}
{
"Action": ["sts:AssumeRole"],
"Effect": "Allow",
"Principal": {"Service": ["states.amazonaws.com"]},
}
],
}
return document
Expand Down
3 changes: 1 addition & 2 deletions samtranslator/model/lambda_.py
Original file line number Diff line number Diff line change
Expand Up @@ -99,8 +99,7 @@ class LambdaEventInvokeConfig(Resource):


class LambdaLayerVersion(Resource):
""" Lambda layer version resource
"""
"""Lambda layer version resource"""

resource_type = "AWS::Lambda::LayerVersion"
property_types = {
Expand Down
42 changes: 20 additions & 22 deletions samtranslator/model/sam_resources.py
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,7 @@


class SamFunction(SamResourceMacro):
"""SAM function macro.
"""
"""SAM function macro."""

resource_type = "AWS::Serverless::Function"
property_types = {
Expand Down Expand Up @@ -697,8 +696,7 @@ def _validate_deployment_preference_and_add_update_policy(


class SamApi(SamResourceMacro):
"""SAM rest API macro.
"""
"""SAM rest API macro."""

resource_type = "AWS::Serverless::Api"
property_types = {
Expand Down Expand Up @@ -810,8 +808,7 @@ def to_cloudformation(self, **kwargs):


class SamHttpApi(SamResourceMacro):
"""SAM rest API macro.
"""
"""SAM rest API macro."""

resource_type = "AWS::Serverless::HttpApi"
property_types = {
Expand Down Expand Up @@ -876,7 +873,13 @@ def to_cloudformation(self, **kwargs):
disable_execute_api_endpoint=self.DisableExecuteApiEndpoint,
)

(http_api, stage, domain, basepath_mapping, route53,) = api_generator.to_cloudformation()
(
http_api,
stage,
domain,
basepath_mapping,
route53,
) = api_generator.to_cloudformation()

resources.append(http_api)
if domain:
Expand All @@ -894,8 +897,7 @@ def to_cloudformation(self, **kwargs):


class SamSimpleTable(SamResourceMacro):
"""SAM simple table macro.
"""
"""SAM simple table macro."""

resource_type = "AWS::Serverless::SimpleTable"
property_types = {
Expand Down Expand Up @@ -954,8 +956,7 @@ def _convert_attribute_type(self, attribute_type):


class SamApplication(SamResourceMacro):
"""SAM application macro.
"""
"""SAM application macro."""

APPLICATION_ID_KEY = "ApplicationId"
SEMANTIC_VERSION_KEY = "SemanticVersion"
Expand All @@ -973,14 +974,12 @@ class SamApplication(SamResourceMacro):
}

def to_cloudformation(self, **kwargs):
"""Returns the stack with the proper parameters for this application
"""
"""Returns the stack with the proper parameters for this application"""
nested_stack = self._construct_nested_stack()
return [nested_stack]

def _construct_nested_stack(self):
"""Constructs a AWS::CloudFormation::Stack resource
"""
"""Constructs a AWS::CloudFormation::Stack resource"""
nested_stack = NestedStack(
self.logical_id, depends_on=self.depends_on, attributes=self.get_passthrough_resource_attributes()
)
Expand All @@ -994,8 +993,7 @@ def _construct_nested_stack(self):
return nested_stack

def _get_application_tags(self):
"""Adds tags to the stack if this resource is using the serverless app repo
"""
"""Adds tags to the stack if this resource is using the serverless app repo"""
application_tags = {}
if isinstance(self.Location, dict):
if self.APPLICATION_ID_KEY in self.Location.keys() and self.Location[self.APPLICATION_ID_KEY] is not None:
Expand All @@ -1009,8 +1007,7 @@ def _get_application_tags(self):


class SamLayerVersion(SamResourceMacro):
""" SAM Layer macro
"""
"""SAM Layer macro"""

resource_type = "AWS::Serverless::LayerVersion"
property_types = {
Expand Down Expand Up @@ -1108,8 +1105,7 @@ def _get_retention_policy_value(self):


class SamStateMachine(SamResourceMacro):
"""SAM state machine macro.
"""
"""SAM state machine macro."""

resource_type = "AWS::Serverless::StateMachine"
property_types = {
Expand All @@ -1125,7 +1121,9 @@ class SamStateMachine(SamResourceMacro):
"Policies": PropertyType(False, one_of(is_str(), list_of(one_of(is_str(), is_type(dict), is_type(dict))))),
"Tracing": PropertyType(False, is_type(dict)),
}
event_resolver = ResourceTypeResolver(samtranslator.model.stepfunctions.events,)
event_resolver = ResourceTypeResolver(
samtranslator.model.stepfunctions.events,
)

def to_cloudformation(self, **kwargs):
managed_policy_map = kwargs.get("managed_policy_map", {})
Expand Down
2 changes: 1 addition & 1 deletion samtranslator/parser/parser.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ def parse(self, sam_template, parameter_values, sam_plugins):

# private methods
def _validate(self, sam_template, parameter_values):
""" Validates the template and parameter values and raises exceptions if there's an issue
"""Validates the template and parameter values and raises exceptions if there's an issue

:param dict sam_template: SAM template
:param dict parameter_values: Dictionary of parameter values provided by the user
Expand Down
8 changes: 5 additions & 3 deletions samtranslator/plugins/application/serverless_app_plugin.py
Original file line number Diff line number Diff line change
Expand Up @@ -294,9 +294,11 @@ def on_after_transform_template(self, template):

# Check each resource to make sure it's active
for application_id, template_id in temp:
get_cfn_template = lambda application_id, template_id: self._sar_client.get_cloud_formation_template(
ApplicationId=self._sanitize_sar_str_param(application_id),
TemplateId=self._sanitize_sar_str_param(template_id),
get_cfn_template = (
lambda application_id, template_id: self._sar_client.get_cloud_formation_template(
ApplicationId=self._sanitize_sar_str_param(application_id),
TemplateId=self._sanitize_sar_str_param(template_id),
)
)
response = self._sar_service_call(get_cfn_template, application_id, application_id, template_id)
self._handle_get_cfn_template_response(response, application_id, template_id)
Expand Down
8 changes: 7 additions & 1 deletion samtranslator/swagger/swagger.py
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,13 @@ def add_lambda_integration(
path_dict[method] = make_conditional(condition, path_dict[method])

def add_state_machine_integration(
self, path, method, integration_uri, credentials, request_templates=None, condition=None,
self,
path,
method,
integration_uri,
credentials,
request_templates=None,
condition=None,
):
"""
Adds aws APIGW integration to the given path+method.
Expand Down
3 changes: 1 addition & 2 deletions samtranslator/translator/translator.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,7 @@


class Translator:
"""Translates SAM templates into CloudFormation templates
"""
"""Translates SAM templates into CloudFormation templates"""

def __init__(self, managed_policy_map, sam_parser, plugins=None):
"""
Expand Down
22 changes: 16 additions & 6 deletions tests/model/test_api_v2.py
Original file line number Diff line number Diff line change
Expand Up @@ -47,13 +47,16 @@ def test_create_authorizer_fails_with_string_authorization_scopes(self):
authorization_scopes="invalid_scope",
)
self.assertEqual(
e.value.message, "Resource with id [logicalId] is invalid. " + "AuthorizationScopes must be a list.",
e.value.message,
"Resource with id [logicalId] is invalid. " + "AuthorizationScopes must be a list.",
)

def test_create_authorizer_fails_with_authorization_scopes_non_oauth2(self):
with pytest.raises(InvalidResourceException) as e:
ApiGatewayV2Authorizer(
api_logical_id="logicalId", name="authName", authorization_scopes=["scope1", "scope2"],
api_logical_id="logicalId",
name="authName",
authorization_scopes=["scope1", "scope2"],
)
self.assertEqual(
e.value.message,
Expand All @@ -67,7 +70,9 @@ def test_create_authorizer_fails_with_authorization_scopes_non_oauth2(self):
def test_create_authorizer_fails_with_jtw_configuration_non_oauth2(self):
with pytest.raises(InvalidResourceException) as e:
ApiGatewayV2Authorizer(
api_logical_id="logicalId", name="authName", jwt_configuration={"config": "value"},
api_logical_id="logicalId",
name="authName",
jwt_configuration={"config": "value"},
)
self.assertEqual(
e.value.message,
Expand All @@ -78,7 +83,9 @@ def test_create_authorizer_fails_with_jtw_configuration_non_oauth2(self):
def test_create_authorizer_fails_with_id_source_non_oauth2(self):
with pytest.raises(InvalidResourceException) as e:
ApiGatewayV2Authorizer(
api_logical_id="logicalId", name="authName", id_source="https://example.com",
api_logical_id="logicalId",
name="authName",
id_source="https://example.com",
)
self.assertEqual(
e.value.message,
Expand Down Expand Up @@ -180,7 +187,8 @@ def test_create_jwt_authorizer_no_id_source(self):
def test_create_lambda_auth_no_function_arn(self):
with pytest.raises(InvalidResourceException) as e:
ApiGatewayV2Authorizer(
api_logical_id="logicalId", name="lambdaAuth",
api_logical_id="logicalId",
name="lambdaAuth",
)
self.assertEqual(
e.value.message,
Expand All @@ -190,7 +198,9 @@ def test_create_lambda_auth_no_function_arn(self):
def test_create_lambda_auth_no_authorizer_payload_format_version(self):
with pytest.raises(InvalidResourceException) as e:
ApiGatewayV2Authorizer(
api_logical_id="logicalId", name="lambdaAuth", function_arn="lambdaArn",
api_logical_id="logicalId",
name="lambdaAuth",
function_arn="lambdaArn",
)
self.assertEqual(
e.value.message,
Expand Down
Loading