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.

Sign up for GitHub

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

Jump to bottom

[AIRFLOW-5129] Add typehint to GCP DLP hook #5980

Merged
merged 1 commit into from
Sep 3, 2019
Merged

[AIRFLOW-5129] Add typehint to GCP DLP hook #5980

merged 1 commit into from
Sep 3, 2019

Conversation

ryanyuan
Copy link
Contributor

@ryanyuan ryanyuan commented Sep 2, 2019 •
edited
Loading

[AIRFLOW-5129] Add typehint to GCP DLP hook

Make sure you have checked all steps below.

Jira

  • My PR addresses the following Airflow-5129 issues and references them in the PR title. For example, "[AIRFLOW-XXX] My Airflow PR"

Description

  • Here are some details about my PR, including screenshots of any UI changes:
    Add typehint to GCP DLP hook airflow/gcp/hooks/dlp.py

Tests

  • My PR adds the following unit tests OR does not need testing for this extremely good reason:

Commits

  • My commits all reference Jira issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":
    1. Subject is separated from body by a blank line
    2. Subject is limited to 50 characters (not including Jira issue reference)
    3. Subject does not end with a period
    4. Subject uses the imperative mood ("add", not "adding")
    5. Body wraps at 72 characters
    6. Body explains "what" and "why", not "how"

Documentation

  • In case of new functionality, my PR adds documentation that describes how to use it.
    • All the public functions and the classes in the PR contain docstrings that explain what it does
    • If you implement backwards incompatible changes, please leave a note in the Updating.md so we can assign it to a appropriate release

Code Quality

  • Passes flake8

@ryanyuan ryanyuan mentioned this pull request Sep 2, 2019
Merged
6 tasks
@mik-laj
Copy link
Member

mik-laj commented Sep 2, 2019

@TobKed Can you do the review?

TobKed
TobKed suggested changes Sep 2, 2019
View reviewed changes
Copy link
Contributor

@TobKed TobKed left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I would require adding Optional when default attribute is None and suggest removing unnecessary line shortening (no value added).

airflow/gcp/hooks/dlp.py Outdated
def __init__(self,
gcp_conn_id="google_cloud_default",
delegate_to=None):
def __init__(self, gcp_conn_id="google_cloud_default", delegate_to=None):
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add type hints here as well?

airflow/gcp/hooks/dlp.py Outdated
):
self,
dlp_job_id: str,
project_id: str = None,
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add Optional to arguments where default is None? There is a discussion about using Optional here [AIRFLOW-5359] Update type annotations in BaseOperator.

Following discussion in python/typing#275, there is a consensus that it is better to require optional types to be made explicit. This PR changes the wording of PEP 484 to allow, but not require, type checkers to treat a None default as implicitly making an argument Optional.

Additional reference: python/peps#689

airflow/gcp/hooks/dlp.py Outdated
name = DlpServiceClient.organization_deidentify_template_path(organization_id, template_id)
name = DlpServiceClient.organization_deidentify_template_path(
organization_id, template_id
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these kind of changes required here (shortening the lines)? Since line pylint max-line-length=110 it should be fine as it was. If you use Black I think you should set line length to 110 as well.

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 think I set it to 79 as PEP-8 suggested. I'll change it to 110 for this project.

Copy link
Contributor

Choose a reason for hiding this comment

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

Black by default is set to 88. This value is not obvious. Here is explanation: https://github.com/psf/black#line-length

airflow/gcp/hooks/dlp.py Outdated
name = DlpServiceClient.organization_deidentify_template_path(organization_id, template_id)
name = DlpServiceClient.organization_deidentify_template_path(
organization_id, template_id
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above.

@mik-laj mik-laj added the provider:google Google (including GCP) related issues label Sep 2, 2019
@ryanyuan
[AIRFLOW-5129] Add typehint to GCP DLP hook
7a2751c 
[AIRFLOW-5129] Add typehint to GCP DLP hook
@ryanyuan
Copy link
Contributor Author

ryanyuan commented Sep 3, 2019

@TobKed All updated.

TobKed
TobKed approved these changes Sep 3, 2019
View reviewed changes
Copy link
Contributor

@TobKed TobKed left a comment

Choose a reason for hiding this comment

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

LGTM. Good job!

@TobKed
Copy link
Contributor

TobKed commented Sep 3, 2019

cc @mik-laj

@mik-laj mik-laj merged commit a9ba915 into apache:master Sep 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mik-laj mik-laj mik-laj approved these changes

@TobKed TobKed TobKed approved these changes

Assignees
No one assigned
Labels
provider:google Google (including GCP) related issues
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ryanyuan @mik-laj @TobKed