Rails 4.2.8? #900

vincemtnz · 2017-03-01T15:45:00Z

Because why not.

Rails 4.2.8 also brings ruby 2.4 support so we can either keep 2.3.3 or update to 2.4.0. In this PR I've taken the 2.4.0 route (was that predictable?) but I'll describe both options:

Staying with ruby 2.3.3
Other than updating the rails gem, I don't think any other changes are necessary, but since we're updating the Gemfile / Gemfile.lock tandem, I'd also take the opportunity to change the github: '...' gems to use git: 'https://...' so we can get rid of the annoying bundler warnings - or am I the only one that's bothered?

With ruby 2.4.0
With 2.4 a few changes are necessary:

Updating all Fixnum / Bignum occurrences to Integer: a5f2189 (see https://bugs.ruby-lang.org/issues/12005)
Update webmock: 25fd407#diff-e79a60dc6b85309ae70a6ea8261eaf95L519
Updated rubocop to a recent version that supports 2.4 (older versions throw an error if an unknown version is found in .ruby-version). This is actually annoying because new rubocop versions introduce new cops like Bundler/OrderedGems which are a bit silly (and we're going to turn off right???). I've autocorrected some rubocop issues 3794b21 and I'm disabling the rest unless they make sense.

osahyoun · 2017-03-02T11:25:24Z

app/models/payment/braintree.rb

+                                     customer: @customer,
+                                     store_in_vault: @store_in_vault
+                                   ).create.id
+                                 end


Rubocop, you naughty machine.

osahyoun · 2017-03-02T11:27:09Z

Vince, thank you for doing this. I love it.

osahyoun · 2017-03-02T11:27:57Z

oh, and I'm totally down with 2.4.0

osahyoun · 2017-03-03T11:59:47Z

Vince, I think we should get this merged. What say you?

rodrei · 2017-03-03T13:03:45Z

app/models/plugins/fundraiser.rb

@@ -38,7 +38,7 @@ def liquid_data(supplemental_data = {})
  end

  def recurring?
-    read_attribute(:recurring_default) > 0
+    read_attribute(:recurring_default).positive?


was this rubocop?

rodrei · 2017-03-03T13:03:59Z

app/models/link.rb

@@ -26,7 +26,7 @@ class Link < ActiveRecord::Base
  private

  def url_has_protocol
-    unless %r{^(https?:)?\/\/}i =~ url
+    unless %r{^(https?:)?\/\/}i.match?(url)


was this rubocop?

rodrei · 2017-03-03T13:06:37Z

app/models/plugins/has_form.rb

@@ -33,7 +33,7 @@ def dup
    clone = super
    clone.save!

-    clone.form.form_elements = form.form_elements.map(&:dup) if clone.form
+    clone.form&.form_elements = form.form_elements.map(&:dup)


these are not equivalent expressions. If clone.form is not present this raises an Undefined method form elements for nil exception. We should keep the guard expression.

Does it though? I'll write a test case to double check, but this seems to work:

irb(main):014:0> house = OpenStruct.new(room: OpenStruct.new(people: 0)) => #<OpenStruct room=#<OpenStruct people=0>> irb(main):019:0> house.room.people = 10 => 10 irb(main):021:0> house.cupboards&.people = 10 => nil irb(main):020:0> house.cupboards.people = 10 NoMethodError: undefined method `people=' for nil:NilClass from (irb):20 from /Users/vincent/.rbenv/versions/2.3.3/bin/irb:11:in `<main>'

That's interesting! I didn't know you could use the safe operator on the left side of an assignment. This means that it's clearly doing more than the old ActiveSupport try.

rodrei · 2017-03-03T13:08:04Z

app/models/share/email.rb

@@ -17,9 +17,9 @@ class Share::Email < ActiveRecord::Base
  include Share::Variant

  validates :subject, :body, presence: true
-  validate :has_link
+  validate :has_link, unless: -> { body.nil? }


is it safe to remove this? This change makes the validation run even when the body is nil.

@rodrei it's an addition.

rodrei · 2017-03-03T13:08:38Z

app/models/share/twitter.rb

@@ -17,9 +17,9 @@ class Share::Twitter < ActiveRecord::Base
  include Share::Variant

  validates :description, presence: true
-  validate :has_link
+  validate :has_link, unless: -> { description.nil? }


same as above

rodrei · 2017-03-03T13:09:39Z

app/services/page_updater.rb

+                  page: @page,
+                  url: @page_url
+                )
+              end


I hate this indentation style 👎 . I vote for us to remove this rubocop check and align it like this:

variant = if condition? do.something else do.something_else end

rodrei · 2017-03-03T13:10:10Z

app/services/search/page_searcher.rb

@@ -7,7 +7,7 @@ def initialize(params)

  def search
    [*@queries].each do |search_type, query|
-      if !validate_query(query)
+      if query.blank?


rodrei · 2017-03-03T13:11:26Z

lib/action_kit/helper.rb

@@ -6,7 +6,7 @@ module Helper
    def check_petition_name_is_available(name)
      resp = ActionKit::Client.get('petitionpage', params: { _limit: 1, name: name })
      if resp.code == 200
-        JSON.parse(resp.body)['meta']['total_count'] == 0
+        (JSON.parse(resp.body)['meta']['total_count']).zero?


is this rubocop?

rodrei · 2017-03-03T13:13:14Z

Other than my comments, this looks awesome! Great job @eyko!

osahyoun · 2017-03-07T11:08:41Z

@eyko sending you a friendly ping.

osahyoun · 2017-03-13T12:45:03Z

This seemed to work fine on staging. Let's try and get it merged before Wednesday - does that sound feasible?

vincemtnz force-pushed the rails-4.2.8 branch 3 times, most recently from cd7fda2 to 762bc5b Compare March 1, 2017 21:10

osahyoun reviewed Mar 2, 2017

View reviewed changes

rodrei reviewed Mar 3, 2017

View reviewed changes

vincemtnz force-pushed the rails-4.2.8 branch from 762bc5b to 9125f77 Compare March 7, 2017 22:16

Vincent Martinez added 3 commits March 13, 2017 13:34

Update to rails 4.2.8 (ruby 2.4.0)

9f7f14e

[rubocop] Run autocorrect and fix other issues

8f32651

Only validate has_link if body/desc is present

e34840a

vincemtnz force-pushed the rails-4.2.8 branch from 9125f77 to e34840a Compare March 13, 2017 13:36

vincemtnz merged commit a037ddf into development Mar 13, 2017

osahyoun deleted the rails-4.2.8 branch March 22, 2018 18:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rails 4.2.8? #900

Rails 4.2.8? #900

vincemtnz commented Mar 1, 2017 •

edited

osahyoun Mar 2, 2017 •

edited

osahyoun commented Mar 2, 2017

osahyoun commented Mar 2, 2017

osahyoun commented Mar 3, 2017

rodrei Mar 3, 2017

rodrei Mar 3, 2017

rodrei Mar 3, 2017

osahyoun Mar 3, 2017

vincemtnz Mar 7, 2017 •

edited

rodrei Mar 10, 2017 •

edited

rodrei Mar 3, 2017

osahyoun Mar 3, 2017

rodrei Mar 3, 2017

rodrei Mar 3, 2017 •

edited

rodrei Mar 3, 2017

rodrei Mar 3, 2017

rodrei commented Mar 3, 2017

osahyoun commented Mar 7, 2017

osahyoun commented Mar 13, 2017

Rails 4.2.8? #900

Rails 4.2.8? #900

Conversation

vincemtnz commented Mar 1, 2017 • edited

osahyoun Mar 2, 2017 • edited

Choose a reason for hiding this comment

osahyoun commented Mar 2, 2017

osahyoun commented Mar 2, 2017

osahyoun commented Mar 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vincemtnz Mar 7, 2017 • edited

Choose a reason for hiding this comment

rodrei Mar 10, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rodrei Mar 3, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rodrei commented Mar 3, 2017

osahyoun commented Mar 7, 2017

osahyoun commented Mar 13, 2017

vincemtnz commented Mar 1, 2017 •

edited

osahyoun Mar 2, 2017 •

edited

vincemtnz Mar 7, 2017 •

edited

rodrei Mar 10, 2017 •

edited

rodrei Mar 3, 2017 •

edited