starhawking / python-terrascript

Create Terraform files using Python scripts.
BSD 2-Clause "Simplified" License
515 stars 76 forks source link

Drop support for Python 3.5 #124

Closed hugovk closed 4 years ago

hugovk commented 4 years ago

Fixes https://github.com/mjuenema/python-terrascript/issues/121.

Much already done in https://github.com/mjuenema/python-terrascript/commit/8448ecf7b5f5d01fd21734552082b28e619ef31c, this also upgrades Python syntax for 3.6+ using https://github.com/asottile/pyupgrade with --py36-plus.

hugovk commented 4 years ago

Passing CI build: https://travis-ci.org/github/mjuenema/python-terrascript/builds/724519337

ilons commented 4 years ago

Looks good, when I run make black I do get a diff however:

index 7a51c96..e7d7ae9 100644
--- a/terrascript/__init__.py
+++ b/terrascript/__init__.py
@@ -99,9 +99,7 @@ class Block(dict):
             raise AttributeError
         else:
             if isinstance(self, Resource):
-                return Attribute(
-                    f"{self.__class__.__name__}.{self._name}.{attr}"
-                )
+                return Attribute(f"{self.__class__.__name__}.{self._name}.{attr}")
             if isinstance(self, Module):
                 return Attribute(f"module.{self._name}.{attr}")
             if isinstance(self, Variable):
@@ -110,9 +108,7 @@ class Block(dict):
                 return Attribute(f"local.{attr}")
             elif isinstance(self, Data):
                 # data.google_compute_image.NAME.ATTR
-                return Attribute(
-                    f"data.{self.__class__.__name__}.{self._name}.{attr}"
-                )
+                return Attribute(f"data.{self.__class__.__name__}.{self._name}.{attr}")
             else:
                 raise AttributeError(attr)

Is this something you get as well when running @hugovk?

hugovk commented 4 years ago

Yes (with Black 19.10b0), updated!

Should the CI fail if there are formatting changes?

With newest Black 20.8b1 there's also some docstring updates. Should I include those?

ilons commented 4 years ago

Yes (with Black 19.10b0), updated!

This is actually an oversight on our end.. Would you mind specifying the exact version required for black in the dev_requirements.txt (pick one you see fit, since the author of Black do not care to mark stable versions)?

Should the CI fail if there are formatting changes?

I think so yes, if you feel like adding that, it would be great, but I'll do it otherwise. There is https://github.com/mjuenema/python-terrascript/issues/127 to track this.

With newest Black 20.8b1 there's also some docstring updates. Should I include those?

If you pin the Black version at the same time, sure thing!

hugovk commented 4 years ago

Pinned Black and updated!

mjuenema commented 4 years ago

Pinned Black and updated!

Thanks for doing that. The Black 20 default behaviour is a little different from Black 19 as I recently discovered when I submitted a Pull Request for some other project. So pinning Black to a specific version is the right thing to do.