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

add changes_file field (adds new mappings) #392

Merged
merged 1 commit into from
Apr 7, 2015

Conversation

andreeap
Copy link
Contributor

This is a fix for #299 and it is ready for review.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 88.68% when pulling a580982 on andreeap:issue-299 into 141117f on CPAN-API:master.

@@ -490,6 +492,25 @@ sub import_tarball {
}
}

sub set_changes_file {
Copy link
Member

Choose a reason for hiding this comment

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

You'll just need to add some logic here to deal with perl distributions, since the Changes file there is perldelta.pod. Have a look at the Changes controller. Basically, once this work is complete and everything is reindexed, we'll want to use this new field rather than all of the logic in that controller.

So basically, you're taking the controller logic and applying it to the changes_file field so that we no longer have to discover where this file is on every request to the Changes controller.

@andreeap
Copy link
Contributor Author

And the field should have the name of the file or the path to the file?

@oalders
Copy link
Member

oalders commented Feb 18, 2015

I think we want the path to the file. In most cases it'll just be the file name anyway, but in Perl's case it could be something like pod/perldelta.pod

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.0%) to 88.58% when pulling 9903041 on andreeap:issue-299 into 141117f on CPAN-API:master.

@oalders
Copy link
Member

oalders commented Feb 20, 2015

@andreeap, there's a perl in our fakepan. Could you test against that as well?

@coveralls
Copy link

Coverage Status

Coverage increased (+0.13%) to 88.87% when pulling c3f85f2 on andreeap:issue-299 into 916b0e3 on CPAN-API:master.

@andreeap
Copy link
Contributor Author

@oalders This is ready for review.

@@ -271,6 +273,34 @@ sub _build_files {
return \@files;
}

sub set_changes_file {
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick here. This should be get_changes_file, since it's only returning the file name and not setting it on the document. The setter is actually $document->changes_file();

@andreeap
Copy link
Contributor Author

I made the changes by adding a second file.

@oalders oalders changed the title add changes_file field add changes_file field (adds new mappings) Mar 24, 2015
@rwstauner
Copy link
Contributor

Looks good, thanks!

@oalders
Copy link
Member

oalders commented Mar 31, 2015

@andreeap could you rebase and fix the merge conflict?

@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) to 88.72% when pulling ea76d49 on andreeap:issue-299 into afddce7 on CPAN-API:master.

oalders added a commit that referenced this pull request Apr 7, 2015
add changes_file field (adds new mappings)
@oalders oalders merged commit 5b059a7 into metacpan:master Apr 7, 2015
@oalders oalders deleted the issue-299 branch April 7, 2015 03:29
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.

None yet

5 participants