Files listed under other files are not authorized #385
Conversation
|
To me, this looks to be ready to merge. @rwstauner any comments? |
| @@ -212,6 +212,13 @@ subtest 'Packages starting with underscore are not indexed' => sub { | |||
| is( $file->module->[0]->indexed, 0, 'Package is not indexed' ); | |||
| }; | |||
|
|
|||
| subtest 'files listed under other files' => sub { | |||
| my $file = new_file_doc( name => 'Makefile.PL' ); | |||
| $file->set_authorized( $file->module => $file->author ); | |||
There was a problem hiding this comment.
These arguments ( $file->module => $file->author ) are confusing as they don't match the sub definition.
They would probably throw an error except that your change returns from the sub before the arguments are used.
Please remove them to avoid someone looking at the code and then wondering the same thing that I just did.
There was a problem hiding this comment.
Oh, after reading the pod for the sub I see that you were perhaps intending to emulate the $perms argument
in which case there are different problems:
$file->module is an array ref (see the previous test) not a module name, and the method accepts a hash ref so you'd need curly braces around the module/author pair, something like
( { $file->module->[0]->{name} => $file->author } ).
Problem there is that these "other" files are unlikely to ever have modules.
I think passing an empty hash ref would suffice: ( {} ).
That way if the code goes past the "other files" check it won't error on not getting a hashref.
Files that are not "other files" should still get authorized then (which would be what you want in this case).
|
I made the change in the test, but I am not sure what to do about the authorized/indexed issue. After I read #323, I understood better the difference between them and now it makes more sense to set indexed => false. |
|
@rwstauner does this look good to you now? |
|
@oalders I was hoping you'd chime in on the "indexed vs authorized" question ;-) |
|
@rwstauner I just had another look #110. You're correct. indexed => false is really what we want here. We don't want these files to show up in searches, so that's clearly the right flag here. |
|
I think this is ready if you can take a look at it. |
|
Looks good. @rwstauner ? |
|
The test passes because the file doesn't contain any content that would get it indexed. The original issue (#110) mentioned a Makefile.PL with some (specific) pod, so that would be a good thing to test. If you add something like this to your subtest: it will expose a small (but familiar) bug in your code. |
|
@rwstauner I did this mistake before, but again I didn't see it because of the test. I hope now it's better. |
| content_cb => sub { \$content } | ||
| ); | ||
|
|
||
| $file->set_indexed( {} ); |
There was a problem hiding this comment.
This lines isn't necessary, new_file_doc calls set_indexed.
|
Glad to see you're learning ;-) |
|
@rwstauner Thanks, I am glad too :) |
|
👍 |
Files listed under other files are not authorized
This is a new pull request for #110 because I cleaned my commit history and rebased and somehow the pull request got closed automatically. This commit is ready for review.