Skip to content

Fix: Corrected position of arguments in _par #2037

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

Merged
merged 2 commits into from
Dec 8, 2022

Conversation

joyaung
Copy link
Contributor

@joyaung joyaung commented Nov 25, 2022

Summary of Changes

  • Swapped order of path and downstream_tables arguments in _parse_lineage function.
  • Added in kwargs when calling function to increase robustness.

Tests

Test suites not relevant for this bug fix, but changes have been tested internally with Neptune server.

Documentation

No documentation to add as there is no change in behavior.

CheckList

Make sure you have checked all steps below to ensure a timely review.

  • PR title addresses the issue accurately and concisely. Example: "Updates the version of Flask to v1.0.2"
  • PR includes a summary of changes.
  • PR adds unit tests, updates existing unit tests, OR documents why no test additions or modifications are needed.
  • 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

@joyaung joyaung requested a review from a team as a code owner November 25, 2022 14:01
@boring-cyborg boring-cyborg bot added area:metadata From the metadata folder category:proxy labels Nov 25, 2022
@boring-cyborg
Copy link

boring-cyborg bot commented Nov 25, 2022

Congratulations on your first Pull Request and welcome to Amundsen community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/amundsen-io/amundsen/blob/main/CONTRIBUTING.md)

@joyaung joyaung force-pushed the fix/gremlin-proxy-lineage-args branch from 561e34c to 9adb61f Compare November 25, 2022 14:05
@joyaung joyaung marked this pull request as draft December 5, 2022 06:28
@joyaung joyaung marked this pull request as ready for review December 5, 2022 06:29
@@ -1958,8 +1958,8 @@ def _get_edge_type_from_user_resource_rel_type(self, relation: UserResourceRel)

raise NotImplementedError(f"Don't know how to handle UserResourceRel={relation}")

def _parse_lineage(self, resource_type: ResourceType, type_: str, upstream_tables: List[LineageItem], path: Path,
downstream_tables: List[LineageItem]) -> Tuple[List[LineageItem], List[LineageItem]]:
def _parse_lineage(self, resource_type: ResourceType, type_: str, upstream_tables: List[LineageItem],
Copy link
Member

Choose a reason for hiding this comment

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

what is the main reason of the argument swap?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previously, parse_lineage function accepted arguments in the wrong order as it was called on line 2038. There are two possible fixes for this:

  1. Align the order of the arguments in parse_lineage and line 2038.
  2. Add kwargs

Technically, either one of the above fixes would work but I have included both.

Signed-off-by: Jovi Loo <[email protected]>
@feng-tao feng-tao merged commit 04fb807 into amundsen-io:main Dec 8, 2022
@boring-cyborg
Copy link

boring-cyborg bot commented Dec 8, 2022

Awesome work, congrats on your first merged pull request!

@joyaung joyaung deleted the fix/gremlin-proxy-lineage-args branch December 8, 2022 08:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:metadata From the metadata folder
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants