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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify example path manipulation with pathlib #77

Merged
merged 4 commits into from
Jan 10, 2023

Conversation

Kilo59
Copy link
Contributor

@Kilo59 Kilo59 commented Dec 9, 2022

Don't need to use both os.path and pathlib.Path

https://treyhunner.com/2019/01/no-really-pathlib-is-great/

I need to double check that great_expectation data_context_root_dir actually can take a pathlib.Path object.
If it doesn't it definitely should 馃槃 .

@denimalpaca
Copy link
Contributor

@Kilo59 Did you ever get a chance to double check that?

@Kilo59
Copy link
Contributor Author

Kilo59 commented Dec 21, 2022

@denimalpaca
So we actually don't currently accept a pathlib.Path but we will soon 馃槄

great-expectations/great_expectations#6613

@Kilo59
Copy link
Contributor Author

Kilo59 commented Jan 6, 2023

@denimalpaca
With the latest release, we do finally accept a pathlib.Path object.
https://github.com/great-expectations/great_expectations/releases/tag/0.15.42

But given that it's so recent it might be better to make this example work on earlier releases.
I'll update this to call str() as the final step to ensure maximum compatibility.

@denimalpaca
Copy link
Contributor

Happy to review and merge when you make that change!

Copy link
Contributor

@denimalpaca denimalpaca left a comment

Choose a reason for hiding this comment

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

Let me know when that string call is added and I'm happy to approve!


ge_root_dir = os.path.join(base_path, "include", "great_expectations")
ge_root_dir = base_path / "include" / "great_expectations"
Copy link
Contributor

Choose a reason for hiding this comment

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

Not entirely sure how base_path works, but are there not +'s missing here?

Copy link
Contributor Author

@Kilo59 Kilo59 Jan 10, 2023

Choose a reason for hiding this comment

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

No. The / operator can be used to join paths.

https://docs.python.org/3/library/pathlib.html#operators

So the following will all produce the same path object. As long as the first item is a Path object.

ge_root_dir = base_path / "include" / "great_expectations"
ge_root_dir = base_path.joinpath("include" , "great_expectations")
ge_root_dir = Path(base_path, "include" , "great_expectations")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@denimalpaca updated.
Also here's another good article explaining pathlib

https://realpython.com/python-pathlib

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the links, looking into that.

@denimalpaca denimalpaca merged commit 99a5b9f into astronomer:main Jan 10, 2023
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.

None yet

3 participants