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

FIX accept pathlib based path as argument for LocalTarget #2548

Merged
merged 13 commits into from Feb 21, 2019

Conversation

orsinium
Copy link
Contributor

@orsinium orsinium commented Oct 8, 2018

Description

Accept pathlib.PosixPath, py._path.local.LocalPath and other path based objects as argument for FileSystemTarget.

Motivation and Context

Fix #2322

Have you tested this? If so, how?

I have included unit tests.

Copy link
Collaborator

@dlstadther dlstadther left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me

luigi/target.py Outdated Show resolved Hide resolved
test/local_target_test.py Outdated Show resolved Hide resolved
@orsinium
Copy link
Contributor Author

orsinium commented Oct 8, 2018

Added fixes from @dlstadther's review.

Copy link
Collaborator

@dlstadther dlstadther 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

@Tarrasch Tarrasch left a comment

Choose a reason for hiding this comment

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

Wow, was it this easy to start supporting it?

Maybe ping @bjlange as the one that opened the ticket in the first place.


By the way, can you look if there's any documentation you can add? Maybe an example usage of the new functionality as class docs for FileSystemTarget?

@bjlange
Copy link

bjlange commented Oct 8, 2018

Amazing! Thanks @orsinium

@orsinium
Copy link
Contributor Author

orsinium commented Oct 9, 2018

Added usage example in docstring.

@orsinium
Copy link
Contributor Author

orsinium commented Nov 5, 2018

What about this PR? Does anyone have any changes requests?

@josham
Copy link
Contributor

josham commented Feb 16, 2019

@Tarrasch @dlstadther Any reason this isn't yet merged?

@dlstadther
Copy link
Collaborator

@josham The code looks good, but the tests aren't passing

@orsinium
Copy link
Contributor Author

I couldn't tame the doctest, so I've just dropped usage example >.<

@orsinium
Copy link
Contributor Author

Travis is ok now

Copy link
Contributor

@Tarrasch Tarrasch left a comment

Choose a reason for hiding this comment

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

I couldn't tame the doctest, so I've just dropped usage example >.<

Really? Why? Can't you just remove one layer of > and it won't run doctest? It still will be super useful to have the docs!

luigi/target.py Outdated Show resolved Hide resolved
@orsinium
Copy link
Contributor Author

Wow, Tarrasch, don't worry about it too much, please. I've returned it as you said. Thank you for a good idea :)

@orsinium
Copy link
Contributor Author

Ok, it doesn't work:
https://travis-ci.org/spotify/luigi/jobs/495852917

@josham
Copy link
Contributor

josham commented Feb 20, 2019

I think you want to remove .. code-block:: python from the docs

@orsinium
Copy link
Contributor Author

In this case it will be broken in the readthedocs api reference.

@Tarrasch
Copy link
Contributor

Just fyi you can test the docs locally too you know: https://github.com/spotify/luigi/blob/master/CONTRIBUTING.rst#writing-documentation

@orsinium
Copy link
Contributor Author

I know. BTW, I've made a few fixes for CONTRIBUTING.rst in another PR. However, sometimes it can't help because I can check locally only "*-core" tests, without hdfs.

I've pushed this code with broken doctest to show that your idea how to bypass this check doesn't work.

@orsinium
Copy link
Contributor Author

now it works

@Tarrasch
Copy link
Contributor

Thanks for this addition @orsinium!

@orsinium
Copy link
Contributor Author

Wow. Thank you, fellows. Sorry for long review process :)

@bjlange
Copy link

bjlange commented Feb 22, 2019

🙌

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

5 participants