Skip to content

Commit e76db5b

Browse files
haarghaarg
committed
clean up diff model to provide more reasonable data to template
The data from the API for diffs is not in the best form. Eventually that should get fixed, but for now move some logic into the model layer rather than the template.
1 parent 9c39c2c commit e76db5b

File tree

3 files changed

+48
-41
lines changed

3 files changed

+48
-41
lines changed

‎lib/MetaCPAN/Web/Controller/Diff.pm

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -11,15 +11,14 @@ sub index : PathPart('diff') : Chained('/') : CaptureArgs(0) {
1111
sub release : Local : Args(4) {
1212
my ( $self, $c, @path ) = @_;
1313
my $diff = $c->model('API::Diff')->releases(@path)->get;
14-
$c->stash(
15-
{ diff => $diff, template => 'diff.html', type => 'release' } );
14+
$c->stash( { diff => $diff, template => 'diff.html' } );
1615
}
1716

1817
sub file : Local : Args(0) {
1918
my ( $self, $c ) = @_;
2019
my $diff = $c->model('API::Diff')
2120
->files( $c->req->params->{source}, $c->req->params->{target} )->get;
22-
$c->stash( { diff => $diff, template => 'diff.html', type => 'source' } );
21+
$c->stash( { diff => $diff, template => 'diff.html' } );
2322
}
2423

2524
__PACKAGE__->meta->make_immutable;

‎lib/MetaCPAN/Web/Model/API/Diff.pm

Lines changed: 32 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -2,33 +2,50 @@ package MetaCPAN::Web::Model::API::Diff;
22
use Moose;
33
use namespace::autoclean;
44
use Digest::SHA ();
5+
use Future;
56

67
extends 'MetaCPAN::Web::Model::API';
78

89
sub releases {
910
my ( $self, @path ) = @_;
10-
return $self->request( '/diff/release/' . join( q{/}, @path ) );
11+
return $self->request( '/diff/release/' . join( q{/}, @path ) )
12+
->then( \&_clean );
1113
}
1214

1315
sub files {
1416
my ( $self, $source, $target ) = @_;
15-
$_ //= '' for $source, $target;
16-
my @source = split( /\//, $source );
17-
$source
18-
= $self->digest( shift @source, shift @source,
19-
join( q{/}, @source ) );
20-
my @target = split( /\//, $target );
21-
$target
22-
= $self->digest( shift @target, shift @target,
23-
join( q{/}, @target ) );
24-
return $self->request( '/diff/file/' . join( q{/}, $source, $target ) );
17+
$source = file_info( $source // '' )->{id};
18+
$target = file_info( $target // '' )->{id};
19+
return $self->request( '/diff/file/' . $source . '/' . $target )
20+
->then( \&_clean );
2521
}
2622

27-
sub digest {
28-
my $self = shift;
29-
my $digest = Digest::SHA::sha1_base64( join( "\0", grep {defined} @_ ) );
23+
sub _clean {
24+
my $diff = shift;
25+
$diff->{$_} = file_info( $diff->{$_} ) for qw(source target);
26+
for my $file ( @{ $diff->{statistics} } ) {
27+
$file->{file} = $file->{source} =~ s{\A(?:[^/]+/){3}}{}r;
28+
delete $file->{source};
29+
delete $file->{target};
30+
( $file->{diff_header} ) = $file->{diff} =~ s/\A(.*?)(^@@)/$2/ms;
31+
}
32+
return Future->done($diff);
33+
}
34+
35+
sub file_info {
36+
my $path = shift;
37+
my ( $author, $release, @parts ) = split m{/}, $path;
38+
my $file_path = join '/', @parts;
39+
my $digest = Digest::SHA::sha1_base64(
40+
join( "\0", grep defined, $author, $release, $file_path ) );
3041
$digest =~ tr/[+\/]/-_/;
31-
return $digest;
42+
return {
43+
author => $author,
44+
release => $release,
45+
( length $file_path ? ( file => $file_path ) : () ),
46+
path => $path,
47+
id => $digest,
48+
};
3249
}
3350

3451
__PACKAGE__->meta->make_immutable;

‎root/diff.html

Lines changed: 14 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
<div class="breadcrumbs">
2-
Diff <a href="/<% type %>/<% diff.source %>"><% diff.source.split("/").first(2).join(" / ") %></a>
2+
Diff <a href="/<% type %>/<% diff.source.path %>"><% diff.source.author %> / <% diff.source.release %></a>
33
&nbsp;/&nbsp;
4-
<a href="/<% type %>/<% diff.target %>"><% diff.target.split("/").first(2).join(" / ") %></a>
4+
<a href="/<% type %>/<% diff.target.path %>"><% diff.target.author %> / <% diff.target.release %></a>
55
</div>
66

77
<%-
@@ -12,36 +12,27 @@
1212
files += 1;
1313
insertions += file.insertions;
1414
deletions += file.deletions;
15-
parts = file.target.split("/");
16-
IF file.target == '/dev/null';
17-
parts = file.source.split("/");
18-
END;
19-
FOREACH i IN [1,2,3]; foo = parts.shift; END;
20-
file.path = parts.join("/"); END -%>
15+
END -%>
2116

2217
<ul class="nav-list slidepanel">
2318
<li class="visible-xs">
2419
<% INCLUDE mobile/toolbar-search-form.html %>
2520
</li>
2621
<li class="nav-header">Tools</li>
27-
<%- IF diff.source.split("/").size != 2 %>
22+
<%- IF diff.source.file %>
2823
<li>
29-
<a href="?source=<% diff.source.split("/").first(2).join("/") %>&target=<% diff.target.split("/").first(2).join("/") %>">
24+
<a href="?source=<% diff.source.author %>/<% diff.source.release %>&amp;target=<% diff.target.author %>/<% diff.target.release %>">
3025
Diff full distribution
3126
</a>
3227
</li>
3328
<% END %>
3429
<li>
35-
<a href="?target=<% diff.source %>&amp;source=<% diff.target %>">
30+
<a href="?source=<% diff.target.path %>&amp;target=<% diff.source.path %>">
3631
Reverse diff
3732
</a>
3833
</li>
3934
<li>
40-
<%- IF type == 'source' %>
41-
<a href="<% api_public %>/diff/file/<% diff.source.digest %>/<% diff.target.digest %>?content-type=text/plain">
42-
<%- ELSE %>
43-
<a href="<% api_public %>/diff/release/<% diff.source %>/<% diff.target %>?content-type=text/plain">
44-
<%- END %>
35+
<a href="<% api_public %>/diff/<% diff.source.file ? 'file' : 'release' %>/<% diff.source.id %>/<% diff.target.id %>?content-type=text/plain">
4536
Raw diff
4637
</a>
4738
</li>
@@ -50,7 +41,7 @@
5041
<select onchange="document.location.href='#' + this.value; this.selectedIndex = 0">
5142
<option>Jump to file</option>
5243
<% FOREACH file IN diff.statistics -%>
53-
<option value="<% file.path %>"><% file.path %></option>
44+
<option value="<% file.file %>"><% file.file %></option>
5445
<% END -%>
5546
</select>
5647
</li>
@@ -65,23 +56,23 @@
6556
<table class="table-striped diff-list">
6657
<% FOREACH file IN diff.statistics %>
6758
<tr>
68-
<td><a href="#<% file.path %>"><% file.path %></a></td>
69-
<td><a href="#<% file.path %>" class="minus"><% file.deletions %></a><a href="#<% file.path %>" class="plus"><% file.insertions %></a></td>
59+
<td><a href="#<% file.file %>"><% file.file %></a></td>
60+
<td><a href="#<% file.file %>" class="minus"><% file.deletions %></a><a href="#<% file.file %>" class="plus"><% file.insertions %></a></td>
7061
</tr>
7162
<% END %>
7263
<tr>
73-
<td><% files %> file<% IF files > 1 %>s<% END %> changed <% IF type == 'source' %> (This is a file diff) <% ELSE %> (This is a version diff) <% END %></td>
64+
<td><% files %> file<% IF files > 1 %>s<% END %> changed <% IF diff.source.file %> (This is a file diff) <% ELSE %> (This is a version diff) <% END %></td>
7465
<td><span class="minus"><% deletions %></span><span class="plus"><% insertions %></span></td>
7566
</tr>
7667
</table>
7768

7869
<% FOREACH file IN diff.statistics %>
79-
<a name="<% file.path %>"></a>
70+
<a name="<% file.file %>"></a>
8071
<div class="diff-container">
8172
<div class="diff-header">
82-
<a href="/source/<% diff.target %>/<% file.path %>"><% file.path %></a>
73+
<a href="/source/<% diff.target.path %>/<% file.file %>"><% file.file %></a>
8374
</div>
84-
<pre><code class="language-diff"><% parts = file.diff.split("\n"); WHILE parts; line = parts.shift; LAST IF line.match( '^\+' ); END; parts.join("\n") %></code></pre>
75+
<pre><code class="language-diff"><% file.diff %></code></pre>
8576
</div>
8677
<% END %>
8778
</div>

0 commit comments

Comments
 (0)