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

Add test cases for the chomp option, fix for undef $/, improve some docs #20

Open
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@HaraldJoerg
Copy link

HaraldJoerg commented Dec 12, 2018

This is a pull request for the CPAN Pull Request Challenge.

The main plan was to add some tests for the chomp option of read_file, which are included. But as ever so often, this lead to other discoveries...

read_file chomps the results only if they have been split into "lines" according to the current value of $/. So I tried some possible values.

read_file has never worked when $/ is undefined, where the split on an empty value would result in an array with two elements per character of input, every other of them undefined. The PR changes this so that with undefined $/, the whole file content will be returned as the first and only element of the array. The test cases no.8 and no.9 exercise that, and fail in the master branch.

Processing fixed length records with e.g. $/ = \1024 doesn't work either, as it tries to split on a regex like SCALAR(0x55c41c65fba0). Apparently nobody has missed it so far, therefore in the current state of the PR I just added to the docs that it isn't supported. If desired, I'd work on it - it isn't too difficult.

Finally, I fixed the docs where it states that the options can be given as a flattened hash, which isn't true for all of the writing operations.

@perhunter

This comment has been minimized.

Copy link
Owner

perhunter commented Dec 12, 2018

@HaraldJoerg

This comment has been minimized.

Copy link

HaraldJoerg commented Dec 12, 2018

I agree! Neither of the settings for $/ provides an interesting use case for File::Slurp. However, it is in these cases where read_file behaves different than plain Perl I/O, different in a way which is impossible to predict without inspecting the code: For example, they return arrays which contain undefined elements. I think that the behavior should either be made similar to Perl I/O (which I did for undefined $/), or at least documented (which I did for integer references), or read_file should even defend itself against these values by throwing an error. I didn't want to throw an error because of the long long list of reverse dependencies of this module. File::Slurp is one of the modules which can easily be used by Perl beginners, who might not yet be aware of the dangers lurking when global variables are changed at some place in the code....

Of course, I'm open to requests for improvement or different decisions by the owners and will adjust the Pull Request as requested.

-- haj

@perhunter

This comment has been minimized.

Copy link
Owner

perhunter commented Dec 12, 2018

@HaraldJoerg

This comment has been minimized.

Copy link

HaraldJoerg commented Dec 12, 2018

read_file only uses the default $/ to split on lines and nothing else. change $/ and you are on your own.

Well, at least paragraph mode ($/ = '') has been supported and documented for a long time.
The docs say that "In list context it will return a list of lines (using the current value of $/ as the separator including support for paragraph mode when it is set to '')."

This documentation can, of course, be adjusted to discourage any current value of $/ different from newline and the empty string, and I'll remove the handling of undefined $/ together with the corresponding tests.

@perhunter

This comment has been minimized.

Copy link
Owner

perhunter commented Dec 12, 2018

@karenetheridge

This comment has been minimized.

Copy link

karenetheridge commented Dec 13, 2018

@perhunter

This comment has been minimized.

Copy link
Owner

perhunter commented Dec 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment