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.

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

Improve global resolvers for missing keys #2983

Closed
wants to merge 6 commits into from
Closed

Conversation

noklam
Copy link
Contributor

@noklam noklam commented Aug 29, 2023

NOTE: Kedro datasets are moving from kedro.extras.datasets to a separate kedro-datasets package in
kedro-plugins repository. Any changes to the dataset implementations
should be done by opening a pull request in that repository.

Description

  • Refactor the variable nested_key
  • remove lambda, use the function, move the default to the underlying function
  • Add a test that should fail (passing now)

Currently when access a non-exist global, a default value is return (I believe this is undesired)

Development notes

Checklist

  • Read the contributing guidelines
  • 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 RELEASE.md file
  • Added tests to cover my changes
  • Checked if this change will affect Kedro-Viz, and if so, communicated that with the Viz team

ankatiyar and others added 6 commits August 25, 2023 13:36
Signed-off-by: Ankita Katiyar <ankitakatiyar2401@gmail.com>
Signed-off-by: Ankita Katiyar <ankitakatiyar2401@gmail.com>
Signed-off-by: Ankita Katiyar <ankitakatiyar2401@gmail.com>
Signed-off-by: Nok <nok.lam.chan@quantumblack.com>
Signed-off-by: Nok <nok.lam.chan@quantumblack.com>
@noklam
Copy link
Contributor Author

noklam commented Aug 29, 2023

I think we should have a sentinel value NOVALUE rather than None to differentiate a user define default None versus a no-op

${globals: DUMMY_KEY, None} should be different from ${globals: DUMMY_KEY}

Base automatically changed from fix-globals to main August 30, 2023 12:45
@astrojuanlu
Copy link
Member

Is this still relevant after merging #2976?

@noklam noklam closed this Aug 31, 2023
@noklam
Copy link
Contributor Author

noklam commented Aug 31, 2023

closing in favor or #2976, I discussed with @ankatiyar and the changes are included in the same PR already.

@astrojuanlu astrojuanlu deleted the nok/fix-globals-pr branch August 31, 2023 08:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants