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

Allow tempdir/tempfile as instance methods #242

Closed
wants to merge 4 commits into from

Conversation

polettix
Copy link
Contributor

@polettix polettix commented Oct 1, 2021

This patch allows using methods tempdir/tempfile as instance methods too, in addition to class methods and simple functions.

When used as instance methods, the underlying instance MUST represent a directory. In this case, it is used to override the DIR option of File::Temp.

The documentation makes a reference to the version when the added behavior is made available, I don't know if this wanted.

This pull request addresses #115

This patch allows using methods tempdir/tempfile as instance methods
too, in addition to class methods and simple functions.

When used as instance methods, the underlying instance MUST represent a
directory. In this case, it is used to override the DIR option of
File::Temp.
@polettix
Copy link
Contributor Author

polettix commented Oct 1, 2021

One thing, to be transparent: any DIR option passed is ignored in this case, i.e. the directory-as-object always wins.

If this not the wanted behavior it's easy (although not trivial) to change this, although in this case I would expect the programmer to use the class method or the exported function.

@xdg
Copy link
Contributor

xdg commented Oct 1, 2021

Thanks. I'm a bit busy with family obligations lately, but I'll try to make time this weekend or next week to review it.

@xdg xdg self-requested a review October 1, 2021 21:14
@polettix
Copy link
Contributor Author

polettix commented Oct 3, 2021

Thanks. I'm a bit busy with family obligations lately, but I'll try to make time this weekend or next week to review it.

No worries and no pressure - it's voluntary open source (and it's not a security bug) 😄

Copy link
Contributor

@xdg xdg left a comment

Choose a reason for hiding this comment

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

This looks great -- I wish I'd done this originally. I have a couple minor requests. Also, I'd like to see a test of the leading options hash to get more coverage of the refactored code paths. (Doing a coverage test on the new code looking for other gaps would be a good idea.)

lib/Path/Tiny.pm Outdated Show resolved Hide resolved
t/temp.t Show resolved Hide resolved
t/temp.t Outdated Show resolved Hide resolved
@polettix
Copy link
Contributor Author

I'll do the requested changes and try to think about additional tests for improving coverage, thanks for looking into it

@xdg xdg closed this in 58725bb Oct 20, 2021
@xdg
Copy link
Contributor

xdg commented Oct 20, 2021

Thank you very much! Shipping a dev release to CPAN now.

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.

2 participants