Add support for passing pathlib.Path objects as filenames #1382

aitorres · 2021-07-10T21:23:23Z

Description of proposed changes

This PR addresses and fixes issue #1381. By adding support to pathlib.Path values that represent file paths to the data_kind helper function, it can be used interchangeably with file name strings on pygmt modules.

Fixes #1381

Reminders

Run make format and make check to make sure the code follows the style guide.
Add tests for new features or tests that would have caught the bug that you're fixing.
Add new public functions/methods/classes to doc/api/index.rst.
Write detailed docstrings for all functions/methods.
If adding new functionality, add an example to docstrings or tutorials.

Slash Commands

You can write slash commands (/command) in the first line of a comment to perform
specific operations. Supported slash commands are:

/format: automatically format and lint the code
/test-gmt-dev: run full tests on the latest GMT development version

welcome · 2021-07-10T21:23:25Z

💖 Thanks for opening this pull request! 💖

Please make sure you read our contributing guidelines and abide by our code of conduct.

A few things to keep in mind:

If you need help writing tests, take a look at the existing ones for inspiration. If you don't know where to start, let us know and we'll walk you through it.
All new features should be documented. It helps to write the docstrings for your functions/classes before writing the code. This will help you think about your code design and results in better code.
No matter what, we are really grateful that you put in the effort to do this! 🎉

weiji14 · 2021-07-13T23:00:29Z

Hmm, supporting pathlib.Path might not be as simple as I thought. We'll need to convert the pathlib.Path object into a str in order for the GMT C API to understand it (the new test_info_path unit test is currently failing at https://github.com/GenericMappingTools/pygmt/pull/1382/checks?check_run_id=3040356636#step:11:589). Do you want to walk through the virtualfile_from_data function at https://github.com/GenericMappingTools/pygmt/blob/master/pygmt/clib/session.py and see if you can find a good place to do the pathlib.Path->str conversion? Or we could change the line at

pygmt/pygmt/helpers/utils.py

Line 106 in 60a92b3

yield arg

to say yield str(arg) perhaps?

aitorres · 2021-07-19T00:56:53Z

I'm sorry for the delay! I've examined the virtualfile_from_data function and modified the conditional statement to cast data to str in presence of "file" data kind, instead of modifying the dummy_context function.

I've pushed my changes to my branch already, and will keep an eye on the tests in case something fails again. Thanks for your help!

weiji14

I'm sorry for the delay! I've examined the virtualfile_from_data function and modified the conditional statement to cast data to str in presence of "file" data kind, instead of modifying the dummy_context function.

Nice one, that's a much better solution! Will see if the tests pass on all platforms (Linux/macOS/Windows).

pygmt/clib/session.py

weiji14

Thanks @aitorres, looks good. I'll merge in 24hrs unless any of the other devs have something to add.

Edit: Please resolve #1382 (comment) before we can proceed.

seisman · 2021-07-19T14:57:08Z

pygmt/helpers/utils.py

        raise GMTInvalidInput("Must provided both x and y.")

-    if isinstance(data, str):
+    if isinstance(data, (str, pathlib.Path)):


What will happen if people use pathlib.PurePath or path.PurePosixPath?

The answer at https://stackoverflow.com/questions/58647584/how-to-test-if-object-is-a-pathlib-path/58966089#58966089 suggests using isinstance(pathlib.PurePath). @aitorres, could you perhaps see if this works?

Suggested change

if isinstance(data, (str, pathlib.Path)):

if isinstance(data, (str, pathlib.PurePath)):

Also, it might be a good idea to test that PureWindowsPath and PurePosixPath objects work on Windows and macOS/Linux respectively. Let us know if you're able to update the unit test to do that. If not, we can also just ignore it.

Gentle ping @aitorres to see if you're still available to finish up this PR! If not, we can get one of the maintainers to finish this up in the next week or so.

Really sorry for the delay! I've just added and pushed the changes to use PurePath, and added two tests that specifically use PureWindowsPath and PurePosixPath.

pygmt/tests/test_info.py

seisman · 2021-08-29T21:50:07Z

Ping @aitorres to finalize this PR if you have time. Otherwise, I'll take over this PR and make edits to this branch.

weiji14

Should be ok to merge after tests all pass, but prefer to have a second approval.

welcome · 2021-09-13T00:09:07Z

🎉🎉🎉 Congrats on merging your first pull request and welcome to the team! 🎉🎉🎉

Please open a new pull request to add yourself to the AUTHORS.md file. We hope that this was a good experience for you. Let us know if there is any way that the contributing process could be improved.

…pingTools#1382) Co-authored-by: Dongdong Tian <[email protected]> Co-authored-by: Wei Ji <[email protected]>

Add support for passing pathlib.Path into pygmt modules

f159418

vercel bot temporarily deployed to Preview July 10, 2021 21:23 Inactive

weiji14 added the enhancement Improving an existing feature label Jul 13, 2021

weiji14 assigned aitorres Jul 13, 2021

Handle pathlib.Path as string file paths for virtual files

32654d3

vercel bot temporarily deployed to Preview July 19, 2021 00:54 Inactive

Update docstring to mention support for pathlib.Path

2cea6da

vercel bot temporarily deployed to Preview July 19, 2021 01:02 Inactive

Fix length of docstring

3bfdb56

weiji14 reviewed Jul 19, 2021

View reviewed changes

pygmt/clib/session.py Outdated Show resolved Hide resolved

vercel bot temporarily deployed to Preview July 19, 2021 01:10 Inactive

Remove unused import

f91b750

vercel bot temporarily deployed to Preview July 19, 2021 01:22 Inactive

weiji14 approved these changes Jul 19, 2021

View reviewed changes

weiji14 added the final review call This PR requires final review and approval from a second reviewer label Jul 19, 2021

seisman reviewed Jul 19, 2021

View reviewed changes

weiji14 removed the final review call This PR requires final review and approval from a second reviewer label Jul 31, 2021

weiji14 added this to the 0.5.0 milestone Aug 11, 2021

aitorres added 2 commits August 22, 2021 18:10

Check for to cover all possible cases of across OSes

367d6e9

Expand tests to support Windows and POSIX Path classes

9dc3fb7

seisman reviewed Aug 23, 2021

View reviewed changes

pygmt/tests/test_info.py Outdated Show resolved Hide resolved

pygmt/tests/test_info.py Outdated Show resolved Hide resolved

pygmt/tests/test_info.py Outdated Show resolved Hide resolved

pygmt/tests/test_info.py Show resolved Hide resolved

Apply suggestions from code review

b3c68ec

vercel bot temporarily deployed to Preview September 9, 2021 23:32 Inactive

seisman added the final review call This PR requires final review and approval from a second reviewer label Sep 10, 2021

Merge branch 'main' into feature/pathlib-support

296be41

vercel bot temporarily deployed to Preview September 12, 2021 08:44 Inactive

weiji14 approved these changes Sep 12, 2021

View reviewed changes

michaelgrund approved these changes Sep 12, 2021

View reviewed changes

seisman changed the title ~~Add support for passing pathlib.Path into pygmt modules~~ Add support for passing pathlib.Path objects as filenames Sep 13, 2021

seisman merged commit 7f53b9d into GenericMappingTools:main Sep 13, 2021

weiji14 removed the final review call This PR requires final review and approval from a second reviewer label Sep 18, 2021

seisman mentioned this pull request Mar 19, 2022

Add a tutorial explaining the generally accepted input types #1268

Open

sixy6e pushed a commit to sixy6e/pygmt that referenced this pull request Dec 21, 2022

Add support for passing pathlib.Path objects as filenames (GenericMap…

3c630f6

…pingTools#1382) Co-authored-by: Dongdong Tian <[email protected]> Co-authored-by: Wei Ji <[email protected]>

seisman mentioned this pull request Oct 16, 2024

clib.Session.virtualfile_in: Refactor if-else statements into match-case statements #3521

Merged

	if isinstance(data, (str, pathlib.Path)):
	if isinstance(data, (str, pathlib.PurePath)):

Add support for passing pathlib.Path objects as filenames #1382

Add support for passing pathlib.Path objects as filenames #1382

Uh oh!

Conversation

aitorres commented Jul 10, 2021 • edited by weiji14 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

welcome bot commented Jul 10, 2021

Uh oh!

weiji14 commented Jul 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aitorres commented Jul 19, 2021

Uh oh!

weiji14 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

weiji14 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seisman Jul 19, 2021

Choose a reason for hiding this comment

Uh oh!

weiji14 Jul 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weiji14 Aug 11, 2021

Choose a reason for hiding this comment

Uh oh!

aitorres Aug 22, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

seisman commented Aug 29, 2021

Uh oh!

weiji14 left a comment

Choose a reason for hiding this comment

Uh oh!

welcome bot commented Sep 13, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

aitorres commented Jul 10, 2021 •

edited by weiji14

Loading

weiji14 commented Jul 13, 2021 •

edited

Loading

weiji14 left a comment •

edited

Loading

weiji14 Jul 21, 2021 •

edited

Loading