Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Add size to versions. #310

Closed
wants to merge 4 commits into from

3 participants

@cmeiklejohn
Collaborator

Add size to versions pushed to gemcutter moving forward.

Addresses issue #303.

app/models/version.rb
@@ -139,6 +139,14 @@ class Version < ActiveRecord::Base
!indexed
end
+ def sized?
+ size.present?
@qrush Owner
qrush added a note

size? will just do this, why is this necessary?

@cmeiklejohn Collaborator

It's not; I'll change it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
app/views/versions/_version.html.erb
@@ -4,6 +4,9 @@
<% if version.platformed? %>
<span class="platform"><%= version.platform %></span>
<% end %>
+ <% if version.sized? %>
+ <span class="size">(<%= version.size_in_kilobytes %>Kb)</span>
@cmeiklejohn Collaborator

Looks good to me, switching it over.

@cmeiklejohn Collaborator

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
app/models/pusher.rb
@@ -76,7 +85,7 @@ class Pusher
def find
@rubygem = Rubygem.find_or_initialize_by_name(spec.name)
- @version = @rubygem.find_or_initialize_version_from_spec(spec)
+ @version = @rubygem.find_or_initialize_version_from_spec_and_size(spec, size)
@qrush Owner
qrush added a note

i dont see why this is necessary, we should set the size but definitely not look up by it

@cmeiklejohn Collaborator

It's not looking up by it, it's looking up by spec, and setting the size. I can rename the function for clarity if you'd like.

def find_or_initialize_version_from_spec_and_size(spec, size)
  version = find_or_initialize_version_from_spec(spec)
  version.size = size
  version
end
@qrush Owner
qrush added a note

Oh, duh. I would just change find_or_initialize_version_from_spec unless if there are other consumers of that method...no need to make a new one if not

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@qrush qrush commented on the diff
app/models/pusher.rb
@@ -11,7 +11,7 @@ class Pusher
end
def process
- pull_spec && find && authorize && save
+ pull_spec && size_gem && find && authorize && save
@qrush Owner
qrush added a note

i'm really disliking this class now, we need to refactor it :(

@cmeiklejohn Collaborator

Which, Pusher?

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

Added commits based on code review. Will rebase and squash if you like once everything looks good.

@cmeiklejohn
Collaborator

Updated find_or_initialize in [be5cee7].

@cldwalker

@qrush Do we want a notification for the false case here? Is there even an actual case where body can be nil after a successful pull_spec?

@cldwalker
Collaborator

@qrush I've rebased and tweaked this branch here. This looks good to go except for my concerns about Pusher#size_gem

@qrush
Owner

How about filling in the sizes for old gems? Can we get a rake task with fog that will make this easy?

@cmeiklejohn
Collaborator

Obsoleted by #484.

@cmeiklejohn cmeiklejohn closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
15 app/models/pusher.rb
@@ -2,7 +2,7 @@ class Pusher
include Vault
include NewRelic::Agent::Instrumentation::ControllerInstrumentation
- attr_reader :user, :spec, :message, :code, :rubygem, :body, :version, :version_id
+ attr_reader :user, :spec, :message, :code, :rubygem, :body, :version, :version_id, :size
def initialize(user, body, host_with_port=nil)
@user = user
@@ -11,7 +11,7 @@ def initialize(user, body, host_with_port=nil)
end
def process
- pull_spec && find && authorize && save
+ pull_spec && size_gem && find && authorize && save
@qrush Owner
qrush added a note

i'm really disliking this class now, we need to refactor it :(

@cmeiklejohn Collaborator

Which, Pusher?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
end
def authorize
@@ -55,6 +55,15 @@ def update
false
end
+ def size_gem
+ if body
+ @size = body.size
+ true
+ else
+ false
+ end
+ end
+
def pull_spec
# Use Gem::Package instead of Gem::Format so that we don't have
# to reread and decode the body of the gem since we only want
@@ -76,7 +85,7 @@ def pull_spec
def find
@rubygem = Rubygem.find_or_initialize_by_name(spec.name)
- @version = @rubygem.find_or_initialize_version_from_spec(spec)
+ @version = @rubygem.find_or_initialize_version_from_spec(spec, size)
if @version.new_record?
true
View
5 app/models/rubygem.rb
@@ -206,9 +206,10 @@ def yank!(version)
end
end
- def find_or_initialize_version_from_spec(spec)
- version = self.versions.find_or_initialize_by_number_and_platform(spec.version.to_s, spec.original_platform.to_s)
+ def find_or_initialize_version_from_spec(spec, size = nil)
+ version = self.versions.find_or_initialize_by_number_and_platform(spec.version.to_s, spec.original_platform.to_s)
version.rubygem = self
+ version.size = size if size
version
end
View
4 app/models/version.rb
@@ -139,6 +139,10 @@ def yanked?
!indexed
end
+ def sized?
+ size?
+ end
+
def info
[ description, summary, "This rubygem does not have a description or summary." ].detect(&:present?)
end
View
3  app/views/versions/_version.html.erb
@@ -4,6 +4,9 @@
<% if version.platformed? %>
<span class="platform"><%= version.platform %></span>
<% end %>
+ <% if version.sized? %>
+ <span class="size">(<%= number_to_human_size(version.size) %>)</span>
+ <% end %>
<% if version.yanked? -%>
<span class="yanked"><%= t '.yanked' %></span>
<% end -%>
View
9 db/migrate/20110710054014_add_size_to_version.rb
@@ -0,0 +1,9 @@
+class AddSizeToVersion < ActiveRecord::Migration
+ def self.up
+ add_column :versions, :size, :integer
+ end
+
+ def self.down
+ remove_column :versions, :size
+ end
+end
View
3  db/schema.rb
@@ -10,7 +10,7 @@
#
# It's strongly recommended to check this file into your version control system.
-ActiveRecord::Schema.define(:version => 20110601153519) do
+ActiveRecord::Schema.define(:version => 20110710054014) do
create_table "announcements", :force => true do |t|
t.text "body"
@@ -142,6 +142,7 @@
t.integer "downloads_count", :default => 0
t.boolean "latest"
t.string "full_name"
+ t.integer "size"
end
add_index "versions", ["built_at"], :name => "index_versions_on_built_at"
View
1  features/push.feature
@@ -11,6 +11,7 @@ Feature: Push Gems
And I visit the gem page for "RGem"
Then I should see "RGem"
And I should see "1.2.3"
+ And I should see "(3 KB)"
Scenario: User pushes existing version of existing gem
Given I am signed up and confirmed as "email@person.com/password"
View
7 lib/tasks/gemcutter.rake
@@ -136,8 +136,7 @@ namespace :gemcutter do
puts "Processing #{local_path}"
begin
cutter = Pusher.new(nil, StringIO.new(File.open(local_path).read))
- cutter.pull_spec
- cutter.write
+ cutter.pull_spec and cutter.size_gem and cutter.write
rescue Exception => e
puts "Problem uploading #{local_path}: #{e}"
end
@@ -155,7 +154,7 @@ namespace :gemcutter do
puts "Processing #{path}"
cutter = Pusher.new(nil, StringIO.new(File.open(path).read))
- cutter.pull_spec and cutter.find and cutter.save
+ cutter.pull_spec and cutter.size_gem and cutter.find and cutter.save
end
end
@@ -170,7 +169,7 @@ namespace :gemcutter do
cutter = Pusher.new(nil, StringIO.new(File.open(path).read))
begin
- cutter.pull_spec and cutter.find and cutter.build
+ cutter.pull_spec and cutter.size_gem and cutter.find and cutter.build
spec_path = File.join(ARGV[1], "#{cutter.rubygem.name}-#{cutter.rubygem.versions.last.to_s}.gem")
if path == spec_path
View
2  public/stylesheets/screen.css
@@ -740,7 +740,7 @@ table {
text-decoration: underline;
}
-.main .info .meta .versions .platform {
+.main .info .meta .versions .platform .size {
color: #B0A58C;
font-size: 0.85em;
margin-left: 4px;
View
1  test/factories/version.rb
@@ -6,6 +6,7 @@
version.platform { "ruby" }
version.association :rubygem
version.indexed true
+ version.size 1024
end
Factory.sequence :version_number do |n|
View
4 test/unit/pusher_test.rb
@@ -38,6 +38,7 @@ class PusherTest < ActiveSupport::TestCase
context "processing incoming gems" do
should "work normally when things go well" do
mock(@cutter).pull_spec { true }
+ mock(@cutter).size_gem { true }
mock(@cutter).find { true }
stub(@cutter).authorize { true }
mock(@cutter).save
@@ -47,6 +48,7 @@ class PusherTest < ActiveSupport::TestCase
should "not attempt to find rubygem if spec can't be pulled" do
mock(@cutter).pull_spec { false }
+ mock(@cutter).size_gem.never
mock(@cutter).find.never
mock(@cutter).authorize.never
mock(@cutter).save.never
@@ -55,6 +57,7 @@ class PusherTest < ActiveSupport::TestCase
should "not attempt to authorize if not found" do
mock(@cutter).pull_spec { true }
+ mock(@cutter).size_gem { true }
mock(@cutter).find { nil }
mock(@cutter).authorize.never
mock(@cutter).save.never
@@ -64,6 +67,7 @@ class PusherTest < ActiveSupport::TestCase
should "not attempt to save if not authorized" do
mock(@cutter).pull_spec { true }
+ mock(@cutter).size_gem { true }
mock(@cutter).find { true }
mock(@cutter).authorize { false }
mock(@cutter).save.never
Something went wrong with that request. Please try again.