Useless assignments fixed, and rails 4 stuff. #1094
7 participants
| @@ -87,7 +87,7 @@ def load_test_users | ||
| suburb_file.pos = 0 if suburb_file.eof? | ||
| row = CSV.parse(suburb_file.readline) | ||
| - suburb, country, state, latitude, longitude = row[0] | ||
| + suburb, _country, _state, latitude, longitude = row[0] |
They indicate that the variable is not going to be used. That's it's okay for linter to ignore this assignment to a useless variable.
it raises the question: Why do we have country and state in the csv file if we don't use it...
@maco: yep, "variable starting with underscore" is a fairly widespread convention for either "private variable" or "variable that can be ignored".
@Br3nda: Good question. This code was merged in #731, and the discussion there doesn't explain where the file suburbs.csv came from. It looks like it's a list of Australian suburbs generated for some other purpose and included wholesale - an Australian government database, perhaps? We should probably know more about its origin, to check we're not causing any licensing issues. @oshiho3, where did that file come from?
Unfortunately, I can't remember where I pulled the suburbs.csv. The purpose of the file was to improve the speed of generating test users. I remember there was a view that shows a number of growers/gardens in bubbles on a map. I think geolocation was normally pulled from external geocoding service when an address was entered in on a user record. Doing this for all test users was causing long wait.
I hope this information helps.
| @@ -49,7 +49,7 @@ def new | ||
| @seed = Seed.new | ||
| # using find_by_id here because it returns nil, unlike find | ||
| - @crop = Crop.find_by_id(params[:crop_id]) || Crop.new | ||
| + @crop = Crop.find_by(id: params[:crop_id]) || Crop.new |
Code Climate is pointing out that alternate_names_controller.rb and ....something else... have practically this same chunk of code in new() and I'm left wondering, given existence of load_and_authorize_resource and load_resource in cancan, do we need to find these by id manually?
Ok, I'm also wondering why we'd be creating a seed for a crop that doesn't exist.
I haven't used cancan much but i thought load_and_authorize_resource would only load the @seed on the seeds controller. The @crop and @owner needs to be loaded by the method that needs it.
Its the
using find_by_id here because it returns nil, unlike find
bit that makes it different.
Can you sum up this? Should it be changed back? Is it okay like this?
Its OK as is (maybe adjust comment), it was more why we can't use load_resource or similar there
|
If I remember right, `load_resource` can be used to get other related
models too. I'll have to check.
…
|
|
i forgot the sorting these last rubocop issues |
|
I vote ignoring those 3 code climate issues about duplication in controllers, at least for now. We do have a comment somewhere suggesting we look at RABL, and https://github.com/plataformatec/responders is also a possibility when tackling "wow, our controllers are just piles of scaffolding" becomes a high priority. |
| @@ -52,7 +52,7 @@ | ||
| # visit /members/unsubscribe/somestring ie.parameter to the URL is a random string | ||
| visit unsubscribe_member_url("type=send_planting_reminder&member_id=#{member.id}") | ||
| expect(page).to have_content "We're sorry, there was an error" | ||
| - updated_member = Member.find(member.id) # reload the member | ||
| + Member.find(member.id) # reload the member |
| @@ -23,7 +23,6 @@ | ||
| description: 'a' * 130 | ||
| ) | ||
| result = helper.display_garden_description(garden) | ||
| - link = link_to("Read more", garden_path(garden)) | ||
| expect(result).to eq 'a' * 130 |
Thats a really weird expectation; like something was lost in refactoring
| @@ -90,7 +90,7 @@ | ||
| context 'ordering' do | ||
| it "should be sorted alphabetically" do | ||
| - z = FactoryGirl.create(:garden_z) | ||
| + FactoryGirl.create(:garden_z) | ||
| a = FactoryGirl.create(:garden_a) |
This one might have been better fixed by fleshing out the other expectation (garden.last == z)
LGTM - its a bit of a mishmash of changes, but the invidual commits make sense.
One or two spots with unused code, and a few other .where(...).first => find_by warnings exist still, would be good to clean that up.
|
Faker is a great gem
…
|
| @@ -196,6 +196,10 @@ def interesting? | ||
| return false | ||
| end | ||
| + def Member.login_name_or_email(login) |
I started with scopes - but the railsguide on how to do scopes says a class method is preferrable to a scope with a variable pass in.
http://guides.rubyonrails.org/active_record_querying.html#passing-in-arguments
However, this is just duplicating the functionality that would be provided to you by a class method.
...
Using a class method is the preferred way to accept arguments for scopes.
|
Still LGTM |
|
I'd like someone who understands the growstuff SSO to test this - there are changes in the warden lookup, so someone checking we're still secure is wise. (I don't use facebook, cos their T&S aren't for me) |
I must have missed that, unless you mean the before_filter => before_action changes. http://stackoverflow.com/questions/16519828/rails-4-before-filter-vs-before-action covers that Did you mean something else? |
|
@clockwerx I mean the query change in find_first_by_auth_conditions |
|
Ah thats all fine. |
Fixes