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

[KED-2165] Remove get_project_context() #324

Merged
merged 6 commits into from
Dec 8, 2020

Conversation

lorenabalan
Copy link
Contributor

@lorenabalan lorenabalan commented Dec 1, 2020

Description

get_project_context was kept for compatibility with Kedro 0.14.*.
Now that we no longer support old versions of Kedro (<0.15), we can replace the calls with load_context() or KedroSession.load_context(). get_project_context will also be removed in future major release of Kedro.

Development notes

  • Dropped support for Kedro 0.15.x.
  • Removed legacy code handling dataset layers (DataCatalog.layers is a definite thing from 0.16.x onwards, no need to check for the old way of having the layer attribute on the dataset class).
  • Removed legacy tests.
  • Ignored covered on branch testing 0.17.0 workflow because that version hasn't been released yet. I could probably test with mocking a lot about but it feels a bit redundant given we'll have to remove that once we release. - Tested this branch manually instead, installing kedro from develop, and changing the __version__ to 0.17.0 (as well as project_version in pyproject.toml in a newly generated kedro project).

QA 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 new entries to the RELEASE.md file
  • Added tests to cover my changes

Legal notice

  • I acknowledge and agree that, by checking this box and clicking "Submit Pull Request":

  • I submit this contribution under the Apache 2.0 license and represent that I am entitled to do so on behalf of myself, my employer, or relevant third parties, as applicable.

  • I certify that (a) this contribution is my original creation and / or (b) to the extent it is not my original creation, I am authorised to submit this contribution on behalf of the original creator(s) or their licensees.

  • I certify that the use of this contribution as authorised by the Apache 2.0 license does not violate the intellectual property rights of anyone else.

@lorenabalan lorenabalan self-assigned this Dec 1, 2020
@lorenabalan lorenabalan marked this pull request as ready for review December 1, 2020 13:56
@lorenabalan lorenabalan marked this pull request as draft December 2, 2020 16:11
@lorenabalan lorenabalan force-pushed the fix/remove-get-project-context branch from acfac69 to 0e962a3 Compare December 2, 2020 16:44
@lorenabalan lorenabalan marked this pull request as ready for review December 3, 2020 09:20
@lorenabalan lorenabalan requested a review from limdauto as a code owner December 3, 2020 09:20
Copy link
Collaborator

@limdauto limdauto left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@DmitriiDeriabinQB DmitriiDeriabinQB left a comment

Choose a reason for hiding this comment

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

LGTM! 🧑‍🍳

package/kedro_viz/server.py Show resolved Hide resolved
package/kedro_viz/server.py Outdated Show resolved Hide resolved
Comment on lines +285 to +287
dataset_to_layer = {}
for layer, dataset_names in _CATALOG.layers.items():
dataset_to_layer.update({dataset_name: layer for dataset_name in dataset_names})
Copy link
Contributor

Choose a reason for hiding this comment

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

🤔 ?

Suggested change
dataset_to_layer = {}
for layer, dataset_names in _CATALOG.layers.items():
dataset_to_layer.update({dataset_name: layer for dataset_name in dataset_names})
dataset_to_layer = {
dataset_name: layer
for layer, dataset_names in _CATALOG.layers.items()
for dataset_name in dataset_names
}

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 find double for-loops in list comprehensions quite hard to read so I prefer to leave it as it was before.

package/kedro_viz/server.py Outdated Show resolved Hide resolved
package/tests/test_server.py Outdated Show resolved Hide resolved
Lorena Bălan and others added 2 commits December 8, 2020 10:17
Co-authored-by: Dmitrii Deriabin <44967953+DmitriiDeriabinQB@users.noreply.github.com>
@lorenabalan lorenabalan force-pushed the fix/remove-get-project-context branch from 7a4c9d6 to 6a482b4 Compare December 8, 2020 11:35
@lorenabalan lorenabalan merged commit add5c71 into main Dec 8, 2020
@lorenabalan lorenabalan deleted the fix/remove-get-project-context branch December 8, 2020 11:48
@richardwestenra richardwestenra mentioned this pull request Dec 16, 2020
1 task
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