Issue340 - MyModule shouldn't be indexed #345
Conversation
|
You could cherry-pick 743b412 to fix the test failure. |
|
Sorry, I'm a little confused. This passes the test suite on my machine, and git log doesn't show me any commits starting with 743b. I'm up to date with master. Did I fork the wrong repository? |
|
It is a bit confusing. :) We're installing Elasticsearch on Travis just before running the tests and the installation gets stuck on a prompt. If you cherry-pick my commit or just copy/paste the change into your .travis.yml and then push your branch, you should see passing tests again. You'll see that other branches are failing on the same issue. This got introduced by a change upstream -- it's not actually specifically a problem on our end. |
|
Added --force-confdef manually. I'm on my way to bed but will check if the build worked first thing tomorrow. Thanks for your help :). |
|
|
||
| =cut | ||
|
|
||
| sub in_test_directory { |
There was a problem hiding this comment.
Since this returns a boolean, it would be nice to name it is_in_test_directory.
|
This looks really good. Sorry again about the wait. If you could just look at my comments and make a couple of changes, I think we're ready to merge this. Thanks very much for adding the tests as well. That's quite helpful. |
|
Thanks! I left the / - it occurs in several other places in the file, so if we're worrying about non-nix systems maybe that should be a separate change. |
| sub is_in_test_directory { | ||
| my $self = shift; | ||
| my @parts = split m{/}, $self->path; | ||
| return ( any { $_ eq 't' } @parts ) ? 1 : 0; |
There was a problem hiding this comment.
This can just be return any { $_ eq 't' } @parts; since any already returns true or false.
|
The '/' is fine. Path::Class is generally a nice way to deal with files and dirs, but in this case I'm not sure it brings much value. I did want to point it out for completeness, though, since it can be quite handy in other situations. |
|
Yeah, I appreciate your pointing it out. Sometimes I forget that windows exists :). The any function seems to return an empty string rather than 0, and the test suite distinguishes between the two. I've changed the tests to expect q{} instead (which looks a little weird to me, but the style checker doesn't like ''). |
|
The empty string brings up an interesting point. From the Test::More docs "You are encouraged to use is() and isnt() over ok() where possible, however do not be tempted to use them to find out if something is true or false!" |
| foreach my $path ( keys %paths ) { | ||
| my $file = MetaCPAN::Document::File->new( %stub, path => $path ); | ||
| my $bool = $paths{$path} ? 'is' : 'is not'; | ||
| is( $file->is_in_test_directory(), |
There was a problem hiding this comment.
Final change here. :) Based on my earlier comment re is vs ok, for a boolean, it's going to be clearer to test for ok( $file->is_in_test_directory() ... ) or ok( ! $file->is_in_test_directory() ...) rather than checking that the false value is specifically an empty string. Since undef or 0 will also evaluate to false, it's going to be clearer if we just check true/false.
|
I realized right after I went to bed last night that there must be something like this :). I put the test cases in two arrays & loop through them separately. I've been rebasing every time I make a change, which is making all these commits look like new ones. Do you normally just rebase once after the patch is ready to merge? |
|
As far as rebasing goes, that just depends. I try to rebase out stuff that doesn't matter just before something is ready to merge, but if something I wrote is terrible and I don't want a record of it, I'll rebase it out sooner. :) It's fine if the commits look like they're new. I review the diff rather than the commits in most cases anyway. It's just nice to clean up your history as you've been doing, since that makes finding problem commits much easier when bugs get hunted later on. |
Issue340 - MyModule shouldn't be indexed
|
\o/ |
This resolves #340.