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

[FEATURE] Accept a pathlib.Path context_root_dir #6613

Merged
merged 27 commits into from
Dec 22, 2022
Merged

[FEATURE] Accept a pathlib.Path context_root_dir #6613

merged 27 commits into from
Dec 22, 2022

Conversation

Kilo59
Copy link
Contributor

@Kilo59 Kilo59 commented Dec 20, 2022 •
edited
Loading

Changes proposed in this pull request:

  • get_context() and other DataContexts can accept a pathlib.Path objects in addition to str paths.

This brings great_expecations in line with the rest of the Python standard library, which since python 3.6 accepts a pathlib.Path or Pathlike object in place of any str filepath representation.

I went with a minimally invasive approach for this and simply convert any pathlib.Path objects to str before assigning to an attribute.

The better, longer-term approach would be to convert all str path representations to a pathlib.Path and pass the richer Path object around.

https://treyhunner.com/2018/12/why-you-should-be-using-pathlib/
https://treyhunner.com/2019/01/no-really-pathlib-is-great/
https://docs.python.org/3/library/pathlib.html#correspondence-to-tools-in-the-os-module

Definition of Done

Please delete options that are not relevant.

  • My code follows the Great Expectations style guide
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added updated unit tests where applicable and made sure that new and existing tests are passing.

@netlify
Copy link

netlify bot commented Dec 20, 2022 •
edited
Loading

✅ Deploy Preview for niobium-lead-7998 ready!

Name Link
🔨 Latest commit c81c0c0
🔍 Latest deploy log https://app.netlify.com/sites/niobium-lead-7998/deploys/63a39457a2bc07000831ec56
😎 Deploy Preview https://deploy-preview-6613--niobium-lead-7998.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@Kilo59 Kilo59 changed the title [FEATURE] Support pathlib.Path context_root_dir [FEATURE] Accept a pathlib.Path context_root_dir Dec 20, 2022
@ghost
Copy link

ghost commented Dec 20, 2022 •
edited by ghost
Loading

👇 Click on the image for a new way to code review

  • Make big changes easier — review code in small groups of related files

  • Know where to start — see the whole change at a glance

  • Take a code tour — explore the change with an interactive tour

  • Make comments and review — all fully sync’ed with github

    Try it now!

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map Legend

@Kilo59 Kilo59 force-pushed the f/1402/core-context-zep-config branch from 3262fc0 to 653010d Compare December 20, 2022 16:06
@Kilo59 Kilo59 marked this pull request as ready for review December 20, 2022 21:18
@Kilo59 Kilo59 enabled auto-merge (squash) December 20, 2022 21:24
@Kilo59 Kilo59 self-assigned this Dec 20, 2022
@Kilo59 Kilo59 added the dx label Dec 20, 2022
@Kilo59 Kilo59 requested review from a team December 21, 2022 13:54
@Kilo59 Kilo59 mentioned this pull request Dec 21, 2022
Closed
6 tasks
@Kilo59 Kilo59 mentioned this pull request Dec 21, 2022
Merged
billdirks
billdirks reviewed Dec 21, 2022
View reviewed changes
Copy link
Contributor

@billdirks billdirks left a comment

Choose a reason for hiding this comment

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

Thanks for doing this. It seems if we always want Union[str, pathlib.Path] for these arguments we should make a TypeAlias to make this a little more concise and so we have a concrete thing to point to if new path arguments get added to the codebase. Could you make this TypeAlias (maybe in great_expectations/types/?) and then use that throughout this PR?

@Kilo59
Copy link
Contributor Author

Kilo59 commented Dec 21, 2022

@billdirks Good idea will do.

Kilo59
Kilo59 commented Dec 21, 2022
View reviewed changes
great_expectations/alias_types.py
@@ -0,0 +1,9 @@
"""This module contains shared TypeAliases"""
Copy link
Contributor Author

@Kilo59 Kilo59 Dec 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

used alias_types.py instead of types.py to avoid weirdness with standard library module names, and to prevent this from being polluted with random types + class definitions.

billdirks
billdirks approved these changes Dec 21, 2022
View reviewed changes
Copy link
Contributor

@billdirks billdirks left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@Kilo59 Kilo59 force-pushed the f/1402/core-context-zep-config branch from 9e6fc05 to fcfbf9b Compare December 21, 2022 22:36
@Kilo59 Kilo59 merged commit b4cc57f into develop Dec 22, 2022
@Kilo59 Kilo59 deleted the f/1402/core-context-zep-config branch December 22, 2022 00:08
Shinnnyshinshin pushed a commit that referenced this pull request Dec 23, 2022
Merge branch 'develop' into f/DX-101/spark-id-pk
7f9770a 
* develop: (31 commits)
  [BUGFIX] Add sqrt on connect to sqlite database. (#6635)
  [FEATURE] Docstring linter for public api (#6638)
  [DOCS] fixing wrong line reference on docs (#6599)
  [MAINTENANCE] CI - install from `requirements-types.txt` (#6639)
  [MAINTENANCE] Unexpected Counts table in DataDocs able to show counts for sampled values (#6634)
  [BUGFIX] Do not round output of proportion computing metrics (#6619)
  [DOCS] - add anonymous_usage_statistics configutation in documentation (#6626)
  [FEATURE] Atomic rendering of meta notes (#6627)
  [MAINTENANCE] Fix develop static type-check stage (#6628)
  [FEATURE] Accept a `pathlib.Path` `context_root_dir` (#6613)
  [FEATURE] F/great 1400/sql datasource (#6623)
  [MAINTENANCE] install `pydantic` in develop pipeline (#6624)
  [MAINTENANCE] Add `row_condition` logic to `RendererConfiguration` and remove from atomic renderer implementations (#6616)
  [MAINTENANCE] create cache as part of `--ci` type-checking step (#6621)
  [FEATURE] Add zep datasource data assistant e2e tests. (#6612)
  [BUGFIX] Simplify metric results processing and improve detection of Decimal types in columns (#6610)
  [BUGFIX] Stop overwriting query with static string in RuntimeBatchRequests for SQL (#6614)
  [MAINTENANCE] update pip installs in pipelines (#6609)
  [FEATURE] Atomic rendering of Evaluation Parameters (#6232)
  [MAINTENANCE] partial `cli` + `usage_stats` typing (#6335)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@billdirks billdirks billdirks approved these changes

@cdkini cdkini Awaiting requested review from cdkini

@NathanFarmer NathanFarmer Awaiting requested review from NathanFarmer

Assignees

@Kilo59 Kilo59

Labels
core-team dx platform
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Kilo59 @billdirks