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

style(datasets): use PEP 526 syntax for variable type annotations #190

Merged
merged 2 commits into from
Apr 26, 2023
Merged

style(datasets): use PEP 526 syntax for variable type annotations #190

merged 2 commits into from
Apr 26, 2023

Conversation

merelcht
Copy link
Member

Description

Same as kedro-org/kedro#2538 but on datasets

Development notes

Checklist

  • Opened this PR as a 'Draft Pull Request' if it is work-in-progress
  • Updated the documentation to reflect the code changes
  • Added a description of this change in the relevant RELEASE.md file
  • Added tests to cover my changes

@merelcht
Use PEP 526 syntax for variable type annotations
d3f7f8f 
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
@merelcht merelcht self-assigned this Apr 25, 2023
@merelcht merelcht requested review from noklam and astrojuanlu April 25, 2023 17:12
astrojuanlu
astrojuanlu reviewed Apr 26, 2023
View reviewed changes
Copy link
Member

@astrojuanlu astrojuanlu left a comment

Choose a reason for hiding this comment

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

Similarly to the Kedro case, we could leverage PEP 585. For now, LGTM!

deepyaman
deepyaman approved these changes Apr 26, 2023
View reviewed changes
Copy link
Member

@deepyaman deepyaman left a comment

Choose a reason for hiding this comment

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

Similarly to the Kedro case, we could leverage PEP 585. For now, LGTM!

Was just about to comment the same suggestion, but only realized it was the same once I looked up the PEP. In any case, LGTM for this PR!

@deepyaman
Copy link
Member

deepyaman commented Apr 26, 2023 •
edited
Loading

@merelcht BTW can you update the branch with main? I feel like it's not requiring branch to be up to date, so not picking up new CI check? Not that your PR title isn't generally nice, but just want to make sure the check is working fine. :)

Possible title post-update: style(datasets): use PEP 526 syntax for variable type annotations

@merelcht merelcht changed the title Use PEP 526 syntax for variable type annotations style(datasets): use PEP 526 syntax for variable type annotations Apr 26, 2023
@merelcht merelcht merged commit 31cf123 into main Apr 26, 2023
@merelcht merelcht deleted the use-pep-526 branch April 26, 2023 14:22
@merelcht merelcht mentioned this pull request Apr 26, 2023
Closed
4 tasks
dannyrfar pushed a commit to dannyrfar/kedro-plugins that referenced this pull request May 3, 2023
@merelcht @dannyrfar
Use PEP 526 syntax for variable type annotations (kedro-org#190)
fdd205c 
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
Signed-off-by: Danny Farah <danny_farah@mckinsey.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@deepyaman deepyaman deepyaman approved these changes

@astrojuanlu astrojuanlu astrojuanlu approved these changes

@noklam noklam Awaiting requested review from noklam

Assignees

@merelcht merelcht

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@merelcht @deepyaman @astrojuanlu