Add support for Path from pathlib.

[Hinidu]

Adding support for Path in file_dep is quite easy. But clearly this is only the part of the problem: if someone (like me) uses Path then he want to use them all other the place. It would be quite strange to pass them to file_dep as is but convert to str before passing in targets for example. I think at least targets should support them too and perhaps it would be not very bad idea to support them in CmdAction.
@schettino72 what do you think about it?

[schettino72]

Yes. Support it everywhere...

I was even thinking about actually storing the PosixPath instead of converting it to string.
I would need to try implementing the code to see exactly what would be the consequences of that change...

[Hinidu]

I was even thinking about actually storing the PosixPath instead of converting it to string.
I would need to try implementing the code to see exactly what would be the consequences of that change...

What's the benefit? I can imagine only two implementations:

1. Store in file_dep both strs and Paths and don't forget to cast items in file_dep when needed - cumbersome and error-prone.
2. Use Paths internally and convert strs provided by the user to them - pointless as I can see because I think doit doesn't use many operations on the file paths besides path comparison, though rich set of handy operations is the main feature of pathlib in my point of view.

But maybe I miss something important.

[schettino72]

What's the benefit?

I was thinking about supporting passing a glob as a file_dep so the glob could be lazily evaluated.
But the glob result is not even an instance of PosixPath, it is just a generator.
So not really much relation to what you are doing...

Another reason would be to have a more robust file comparison. See #109, but I guess that is not worth the problem.

Conclusion: you are right 😄 , just convert to string.

[schettino72]

• You need to add a dependency to pathlib on setup.py because doit still supports python3.3.
• Note some test failures on windows, check appveyor

[Hinidu]

I was thinking about supporting passing a glob as a file_dep so the glob could be lazily evaluated.
But the glob result is not even an instance of PosixPath, it is just a generator.
So not really much relation to what you are doing...

It also can be a good thing to do, but I agree that not in this PR.

Another reason would be to have a more robust file comparison. See #109, but I guess that is not worth the problem.

Hmm, I didn't think about some_file and ./some_file. I thought only about case-insensitive file systems.
Now it looks more benefitial. I will try to investigate the converting of all user strings to Paths - maybe it can be done as the next step after accepting Paths from the user.

Conclusion: you are right 😄 , just convert to string.

Yeah, I think it would be a good first step :-)

You need to add a dependency to pathlib on setup.py because doit still supports python3.3.
Note some test failures on windows, check appveyor

Ok.

[Hinidu]

Note some test failures on windows, check appveyor

It looks like these failed tests didn't run before this PR on Windows. I assumed it by the number of tests:

• PR: ============== 4 failed, 702 passed, 15 skipped in 13.44 seconds ==============
• master: =================== 699 passed, 21 skipped in 13.94 seconds ===================

I have added only one test.
These failed tests have a decorator: @pytest.mark.skipif("os.system('strace -V') != 0")
Do you have any ideas why strace -V could change its behaviour on AppVeyor?

[Hinidu]

@schettino72 I've commited support for Path in targets and CmdAction. Waiting for your comments on implementation. Could you point to the specific parts of the documentation which I should update to reflect these changes?

[Hinidu]

Perhaps it would be reasonable to do type checking for targets and CmdAction similar to existing for file_dep to provide more meaningful error message to the user.
Current approach will convert to str anything user will provide. I can imagine that some users want from time to time put some ints in CmdAction, but I think that other types in these places much rather would be a mistake.
What do you think about it?

[schettino72]

Do you have any ideas why strace -V could change its behaviour on AppVeyor?

Sorry. No idea.

Could you point to the specific parts of the documentation which I should update to reflect these changes?

When it introduces file_dep add an example using pathlib. Then in other places (targets, ...) just mention that it also supports pathlib too.

Perhaps it would be reasonable to do type checking for targets and CmdAction similar to existing for file_dep to provide more meaningful error message to the user.

go ahead 😄

Sorry I didnt review the code yet... and probably wont do that next week also 😓

[schettino72]

Do you have any ideas why strace -V could change its behaviour on AppVeyor?

I re-run the tests in an old commit just to double check.
Now AppVeyor provides a strace (from cygwin).
https://ci.appveyor.com/project/schettino72/doit/build/1.0.97/job/qlx1air685fiaxvs#L223

I dont want to debug that... lets just disable the test if strace not available OR it is on windows.
If you dont want to do this change yourself, it is fine... but please create another issue to track this. thanks.

[schettino72]

thanks. one more thing. add your name in the AUTHORS file 😀

[schettino72]

and an entry on CHANGES file

[schettino72]

test_file_dep_str()

[Hinidu]

Fixed

[schettino72]

looks good. thanks. just need docs now.

[Hinidu]

thanks. one more thing. add your name in the AUTHORS file 😀

I'm already there 😉

I'll add info to CHANGES and docs.

[Hinidu]

@schettino72 thank you for review.
I've pushed requested changes, type checking in targets and CmdAction and docs update.
PR looks ready to me now.

[schettino72]

thanks. squashed and merged.

There was a typo in the docs, already fixed.
Next time you can check for typos using doit spell.

[schettino72]

actually I would prefer to have a separate example... to keep examples really simple.
But I merged anyway.

[Hinidu]

I agree - it is a good thing to concentrate reader on one feature at a time. I can remove this example here and add a new section like pathlib support with example which uses pathlib both in file_dep, targets and CmdAction.

[schettino72]

PR with whatever improvements are always welcome.

[Hinidu]

@schettino72 thank you for merging!
By the way this PR can help partially solve the problem from #109 if user will provide Path instances instead of strings. str(Path('./afile')) == str(Path('afile')) == 'afile'. But it doesn't resolve symbolic links and .. though.

[schettino72]

By the way this PR can help partially solve the problem from #109 if user will provide Path instances instead of strings. str(Path('./afile')) == str(Path('afile')) == 'afile'. But it doesn't resolve symbolic links and .. though.

To tell you the truth I dont care about this feature. I am worried it could have a negative effect on performance. And I dont know how I would implement that. Anyway if you want to give a try, go ahead :)