Repository: google/cloud-forensics-utils

Issue:
Be consistent in type hinting named parameters
Ideally we would have `def foo(bar: Optional[Type] = None) -> ReturnType` for named parameters which default to `None` instead of  `def foo(bar: Type = None) -> ReturnType`. As of now we have both across the codebase.

Note: we want to use `Optional` where it makes sense from `mypy` POV. `def foo(bar: Optional[str] = 'default-string') -> ReturnType` does not make sense because `Optional` essentially means `Union[type, None]`. In this case this would mean `Union[str, None]` and unless we're specifically allowing / expecting that `bar` may be `None`, `Optional` shouldn't be used here.

Discussion:
codecov-commenter: # [Codecov](https://codecov.io/gh/google/cloud-forensics-utils/pull/186?src=pr&el=h1) Report
> Merging [#186](https://codecov.io/gh/google/cloud-forensics-utils/pull/186?src=pr&el=desc) into [master](https://codecov.io/gh/google/cloud-forensics-utils/commit/e4448c60002eee03c344ec2803276ff5d91936be&el=desc) will **decrease** coverage by `0.20%`.
> The diff coverage is `66.90%`.

[![Impacted file tree graph](https://codecov.io/gh/google/cloud-forensics-utils/pull/186/graphs/tree.svg?width=650&height=150&src=pr&token=zZkPno3ah0)](https://codecov.io/gh/google/cloud-forensics-utils/pull/186?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #186      +/-   ##
==========================================
- Coverage   65.39%   65.19%   -0.21%     
==========================================
  Files          14       16       +2     
  Lines         968     1198     +230     
==========================================
+ Hits          633      781     +148     
- Misses        335      417      +82     
```

| Flag | Coverage Δ | |
|---|---|---|
| #nosetests | `65.19% <66.90%> (-0.21%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/google/cloud-forensics-utils/pull/186?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [libcloudforensics/providers/gcp/forensics.py](https://codecov.io/gh/google/cloud-forensics-utils/pull/186/diff?src=pr&el=tree#diff-bGliY2xvdWRmb3JlbnNpY3MvcHJvdmlkZXJzL2djcC9mb3JlbnNpY3MucHk=) | `51.72% <40.00%> (-13.28%)` | :arrow_down: |
| [libcloudforensics/providers/aws/forensics.py](https://codecov.io/gh/google/cloud-forensics-utils/pull/186/diff?src=pr&el=tree#diff-bGliY2xvdWRmb3JlbnNpY3MvcHJvdmlkZXJzL2F3cy9mb3JlbnNpY3MucHk=) | `60.00% <43.75%> (-9.77%)` | :arrow_down: |
| [...ibcloudforensics/providers/gcp/internal/compute.py](https://codecov.io/gh/google/cloud-forensics-utils/pull/186/diff?src=pr&el=tree#diff-bGliY2xvdWRmb3JlbnNpY3MvcHJvdmlkZXJzL2djcC9pbnRlcm5hbC9jb21wdXRlLnB5) | `54.88% <48.57%> (-7.48%)` | :arrow_down: |
| [libcloudforensics/providers/aws/internal/log.py](https://codecov.io/gh/google/cloud-forensics-utils/pull/186/diff?src=pr&el=tree#diff-bGliY2xvdWRmb3JlbnNpY3MvcHJvdmlkZXJzL2F3cy9pbnRlcm5hbC9sb2cucHk=) | `73.07% <55.55%> (-4.20%)` | :arrow_down: |
| [...ibcloudforensics/providers/gcp/internal/project.py](https://codecov.io/gh/google/cloud-forensics-utils/pull/186/diff?src=pr&el=tree#diff-bGliY2xvdWRmb3JlbnNpY3MvcHJvdmlkZXJzL2djcC9pbnRlcm5hbC9wcm9qZWN0LnB5) | `63.63% <56.25%> (-2.08%)` | :arrow_down: |
| [...cs/providers/gcp/internal/compute\_base\_resource.py](https://codecov.io/gh/google/cloud-forensics-utils/pull/186/diff?src=pr&el=tree#diff-bGliY2xvdWRmb3JlbnNpY3MvcHJvdmlkZXJzL2djcC9pbnRlcm5hbC9jb21wdXRlX2Jhc2VfcmVzb3VyY2UucHk=) | `38.23% <63.63%> (+1.72%)` | :arrow_up: |
| [libcloudforensics/providers/gcp/internal/common.py](https://codecov.io/gh/google/cloud-forensics-utils/pull/186/diff?src=pr&el=tree#diff-bGliY2xvdWRmb3JlbnNpY3MvcHJvdmlkZXJzL2djcC9pbnRlcm5hbC9jb21tb24ucHk=) | `59.22% <75.00%> (+2.90%)` | :arrow_up: |
| [libcloudforensics/providers/aws/internal/ec2.py](https://codecov.io/gh/google/cloud-forensics-utils/pull/186/diff?src=pr&el=tree#diff-bGliY2xvdWRmb3JlbnNpY3MvcHJvdmlkZXJzL2F3cy9pbnRlcm5hbC9lYzIucHk=) | `61.76% <77.77%> (+0.47%)` | :arrow_up: |
| [...ibcloudforensics/providers/gcp/internal/storage.py](https://codecov.io/gh/google/cloud-forensics-utils/pull/186/diff?src=pr&el=tree#diff-bGliY2xvdWRmb3JlbnNpY3MvcHJvdmlkZXJzL2djcC9pbnRlcm5hbC9zdG9yYWdlLnB5) | `80.00% <80.00%> (ø)` | |
| [libcloudforensics/providers/gcp/internal/build.py](https://codecov.io/gh/google/cloud-forensics-utils/pull/186/diff?src=pr&el=tree#diff-bGliY2xvdWRmb3JlbnNpY3MvcHJvdmlkZXJzL2djcC9pbnRlcm5hbC9idWlsZC5weQ==) | `80.85% <83.33%> (+50.85%)` | :arrow_up: |
| ... and [9 more](https://codecov.io/gh/google/cloud-forensics-utils/pull/186/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/google/cloud-forensics-utils/pull/186?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/google/cloud-forensics-utils/pull/186?src=pr&el=footer). Last update [5039697...36d6e44](https://codecov.io/gh/google/cloud-forensics-utils/pull/186?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

giovannt0: @tomchop if you have the following:

```
def foo(arg1: Optional[int] = 5) -> int:  # arg1 is optional, therefore could be type hinted as such.
    return bar(arg1)  # mypy will complain here, because we're passing an Optional[int] to a method that's type hinted as 
                      # having its first parameter being an int, not an optional one.

def bar(arg1: int) -> int:
    return arg1
```

From what we tested with @sa3eed3ed, either we have not to type hint some parameters as `Optional` (which ends up confusing people reading through the codebase), or we have to use `--no-strict-optional`.
giovannt0: As discussed: dropping commit `fa005f` that added more `Optional` annotations. Un-annotated parameters will stay this way to stay consistent with what `Optional` means to `mypy` rather to what it looks like it means from the user's perspective. This will be documented in upcoming doc release.
@sa3eed3ed @tomchop PTAL for the review.

Patch:
diff --git a/examples/aws_cli.py b/examples/aws_cli.py
index e9924af..089aedf 100644
--- a/examples/aws_cli.py
+++ b/examples/aws_cli.py
@@ -149,6 +149,7 @@ def StartAnalysisVm(args: 'argparse.Namespace') -> None:
                                                             str(vm[1]),
                                                             vm[0].region))
 
+
 def ListImages(args: 'argparse.Namespace') -> None:
   """List AMI images and filter on AMI image 'name'.
 
@@ -157,7 +158,7 @@ def ListImages(args: 'argparse.Namespace') -> None:
   """
   aws_account = account.AWSAccount(args.zone)
 
-  qfilter = [{'Name':'name', 'Values':[args.filter]}]
+  qfilter = [{'Name': 'name', 'Values': [args.filter]}]
 
   images = aws_account.ListImages(qfilter)
 
diff --git a/libcloudforensics/providers/aws/internal/account.py b/libcloudforensics/providers/aws/internal/account.py
index 244a2f8..3cacbdf 100644
--- a/libcloudforensics/providers/aws/internal/account.py
+++ b/libcloudforensics/providers/aws/internal/account.py
@@ -745,9 +745,9 @@ class AWSAccount:
     block_device_mapping['Ebs']['VolumeSize'] = boot_volume_size
     return block_device_mapping
 
-  # pylint: disable=line-too-long
-  def ListImages(self,
-                 qfilter: Optional[List[Dict[str, Any]]]) -> List[Dict[str, Any]]:
+  def ListImages(
+      self,
+      qfilter: Optional[List[Dict[str, Any]]] = None) -> List[Dict[str, Any]]:
     """List AMI images.
 
     Args:
@@ -760,12 +760,14 @@ class AWSAccount:
     Raises:
       RuntimeError: If the images could not be listed.
     """
+    if not qfilter:
+      qfilter = []
 
     client = self.ClientApi(common.EC2_SERVICE)
     try:
-      images = client.describe_images(Filters=qfilter) # type: Dict[str, List[Dict[str, Any]]]
+      images = client.describe_images(
+          Filters=qfilter)  # type: Dict[str, List[Dict[str, Any]]]
     except client.exceptions.ClientError as exception:
       raise RuntimeError(str(exception))
 
     return images['Images']
-  # pylint: enable=line-too-long
diff --git a/libcloudforensics/providers/gcp/internal/compute.py b/libcloudforensics/providers/gcp/internal/compute.py
index 02108fe..37be05d 100644
--- a/libcloudforensics/providers/gcp/internal/compute.py
+++ b/libcloudforensics/providers/gcp/internal/compute.py
@@ -54,8 +54,8 @@ class GoogleCloudCompute(common.GoogleCloudComputeClient):
     super(GoogleCloudCompute, self).__init__(self.project_id)
 
   def Instances(self,
-                refresh: Optional[bool] = True
-               ) -> Dict[str, 'GoogleComputeInstance']:
+                refresh: bool = True
+                ) -> Dict[str, 'GoogleComputeInstance']:
     """Get all instances in the project.
 
     Args:
@@ -71,8 +71,8 @@ class GoogleCloudCompute(common.GoogleCloudComputeClient):
     return self._instances  # type: ignore
 
   def Disks(self,
-            refresh: Optional[bool] = True
-           ) -> Dict[str, 'GoogleComputeDisk']:
+            refresh: bool = True
+            ) -> Dict[str, 'GoogleComputeDisk']:
     """Get all disks in the project.
 
     Args:
@@ -185,8 +185,8 @@ class GoogleCloudCompute(common.GoogleCloudComputeClient):
       self,
       snapshot: 'GoogleComputeSnapshot',
       disk_name: Optional[str] = None,
-      disk_name_prefix: Optional[str] = '',
-      disk_type: Optional[str] = 'pd-standard') -> 'GoogleComputeDisk':
+      disk_name_prefix: str = '',
+      disk_type: str = 'pd-standard') -> 'GoogleComputeDisk':
     """Create a new disk based on a Snapshot.
 
     Args:
@@ -238,12 +238,12 @@ class GoogleCloudCompute(common.GoogleCloudComputeClient):
   def GetOrCreateAnalysisVm(self,
                             vm_name: str,
                             boot_disk_size: int,
-                            disk_type: Optional[str] = 'pd-standard',
-                            cpu_cores: Optional[int] = 4,
-                            image_project: Optional[str] = 'ubuntu-os-cloud',
-                            image_family: Optional[str] = 'ubuntu-1804-lts',
+                            disk_type: str = 'pd-standard',
+                            cpu_cores: int = 4,
+                            image_project: str = 'ubuntu-os-cloud',
+                            image_family: str = 'ubuntu-1804-lts',
                             packages: Optional[List[str]] = None
-                           ) -> Tuple['GoogleComputeInstance', bool]:
+                            ) -> Tuple['GoogleComputeInstance', bool]:
     """Get or create a new virtual machine for analysis purposes.
 
     If none of the optional parameters are specified, then by default the
@@ -346,8 +346,8 @@ class GoogleCloudCompute(common.GoogleCloudComputeClient):
 
   def ListInstanceByLabels(self,
                            labels_filter: Dict[str, str],
-                           filter_union: Optional[bool] = True
-                          )-> Dict[str, 'GoogleComputeInstance']:
+                           filter_union: bool = True
+                           )-> Dict[str, 'GoogleComputeInstance']:
     """List VMs in a project with one/all of the provided labels.
 
     This will call the _ListByLabel function on an instances() API object
@@ -372,8 +372,8 @@ class GoogleCloudCompute(common.GoogleCloudComputeClient):
 
   def ListDiskByLabels(self,
                        labels_filter: Dict[str, str],
-                       filter_union: Optional[bool] = True
-                      ) -> Dict[str, 'GoogleComputeDisk']:
+                       filter_union: bool = True
+                       ) -> Dict[str, 'GoogleComputeDisk']:
     """List Disks in a project with one/all of the provided labels.
 
     This will call the _ListByLabel function on a disks() API object
@@ -551,9 +551,9 @@ class GoogleCloudCompute(common.GoogleCloudComputeClient):
   def ImportImageFromStorage(self,
                              storage_image_path: str,
                              image_name: Optional[str] = None,
-                             bootable: Optional[bool] = False,
+                             bootable: bool = False,
                              os_name: Optional[str] = None,
-                             guest_environment: Optional[bool] = True) -> 'GoogleComputeImage':  # pylint: disable=line-too-long
+                             guest_environment: bool = True) -> 'GoogleComputeImage':  # pylint: disable=line-too-long
     """Import GCE image from Cloud storage.
 
     The import tool supports raw disk images and most virtual disk
@@ -567,9 +567,9 @@ class GoogleCloudCompute(common.GoogleCloudComputeClient):
           in "%Y%m%d%H%M%S" format.
       bootable (bool): Optional. True if the imported image is bootable.
           Default is False. If True the os_name must be specified.
-      os_name (str): Optional. Name of the operating system on the bootable image.
-          For supported versions please see:
-          https://cloud.google.com/sdk/gcloud/reference/compute/images/import#--os
+      os_name (str): Optional. Name of the operating system on the bootable
+          image. For supported versions please see:
+          https://cloud.google.com/sdk/gcloud/reference/compute/images/import#--os  # pylint: disable=line-too-long
           For known limitations please see:
           https://googlecloudplatform.github.io/compute-image-tools/image-import.html#compatibility-and-known-limitations  # pylint: disable=line-too-long
       guest_environment (bool): Optional. Install Google Guest Environment on a
@@ -747,7 +747,7 @@ class GoogleComputeInstance(compute_base_resource.GoogleComputeBaseResource):
 
   def AttachDisk(self,
                  disk: 'GoogleComputeDisk',
-                 read_write: Optional[bool] = False) -> None:
+                 read_write: bool = False) -> None:
     """Attach a disk to the virtual machine.
 
     Args:
diff --git a/libcloudforensics/providers/gcp/internal/compute_base_resource.py b/libcloudforensics/providers/gcp/internal/compute_base_resource.py
index f948f72..144647e 100644
--- a/libcloudforensics/providers/gcp/internal/compute_base_resource.py
+++ b/libcloudforensics/providers/gcp/internal/compute_base_resource.py
@@ -163,7 +163,7 @@ class GoogleComputeBaseResource(common.GoogleCloudComputeClient):
 
   def AddLabels(self,
                 new_labels_dict: Dict[str, Any],
-                blocking_call: Optional[bool] = False) -> Optional[Any]:
+                blocking_call: bool = False) -> Optional[Any]:
     """Add or update labels of a compute resource.
 
     Args:
diff --git a/libcloudforensics/providers/gcp/internal/storage.py b/libcloudforensics/providers/gcp/internal/storage.py
index 3d21806..b1eb393 100644
--- a/libcloudforensics/providers/gcp/internal/storage.py
+++ b/libcloudforensics/providers/gcp/internal/storage.py
@@ -20,6 +20,7 @@ from libcloudforensics.providers.gcp.internal import common
 if TYPE_CHECKING:
   import googleapiclient
 
+
 def SplitGcsPath(gcs_path: str) -> Tuple[str, str]:
   """Split GCS path to bucket name and object URI.
 


Test patch:
diff --git a/tests/providers/aws/aws_e2e.py b/tests/providers/aws/aws_e2e.py
index 922d8cf..d9e02df 100644
--- a/tests/providers/aws/aws_e2e.py
+++ b/tests/providers/aws/aws_e2e.py
@@ -124,7 +124,7 @@ class EndToEndTest(unittest.TestCase):
     """
 
     aws_account = account.AWSAccount(self.zone)
-    qfilter = [{'Name':'name', 'Values':['Ubuntu 18.04*']}]
+    qfilter = [{'Name': 'name', 'Values': ['Ubuntu 18.04*']}]
     images = aws_account.ListImages(qfilter)
 
     self.assertGreater(len(images), 0)
diff --git a/tests/providers/aws/aws_test.py b/tests/providers/aws/aws_test.py
index b81291c..0844226 100644
--- a/tests/providers/aws/aws_test.py
+++ b/tests/providers/aws/aws_test.py
@@ -438,10 +438,11 @@ class AWSAccountTest(unittest.TestCase):
     """Test that AMI images are correctly listed."""
     describe_images = mock_ec2_api.return_value.describe_images
     describe_images.return_value = MOCK_DESCRIBE_IMAGES
-    images = FAKE_AWS_ACCOUNT.ListImages(qfilter=None)
+    images = FAKE_AWS_ACCOUNT.ListImages()
     self.assertEqual(2, len(images))
     self.assertIn('Name', images[0])
 
+
 class AWSInstanceTest(unittest.TestCase):
   """Test AWSInstance class."""
   # pylint: disable=line-too-long