Rename variables and remove unused code in fixtures.rb #4254

Merged
merged 3 commits into from May 10, 2012

4 participants

@alexeymuranov

Rename local variables and parameters to have more consistent names. In particular, instances of ActiveRecord::Fixtures be called fixture_set's.

Remove unused private method yaml_fixtures_key.

Change arity of instantiate_fixtures by removing an unused parameter. (Tests are passing.)

@alexeymuranov alexeymuranov commented on the diff Jan 1, 2012
activerecord/lib/active_record/fixtures.rb
@@ -423,7 +426,7 @@ def self.cache_fixtures(connection, fixtures_map)
cache_for_connection(connection).update(fixtures_map)
end
- def self.instantiate_fixtures(object, fixture_name, fixtures, load_instances = true)

fixture_name is not used.

@jeremy
Ruby on Rails member
jeremy added a note May 7, 2012

Not used by Rails, but this is public API. Changing args needs deprecation.

Thank you for looking at all this, i can work on deprecating it properly, but i would appreciate some advise, i have not done this before.

What is the recommended way to deprecate methods: with Module.deprecate or with ActiveSupport::Deprecation.deprecate_methods?

Is there some online guide? I've found this so far:
http://8raystech.com/2011/02/07/how-to-deprecate-methods-and-constants-in-rails-3

Here is my attempt to deprecate the arity of instantiate_fixtures.

@jeremy
Ruby on Rails member
jeremy added a note May 8, 2012

You can change the method signature on master. Just need to deprecate on the 3-2-stable branch :)

Your approach (checking for boolean arg) looks good though. Think it'd be clearer as a single method, though (no __with_new_arity)

How about merging this into master first, then deprecating in 3-2-stable, and then removing the old method from master?

@jeremy
Ruby on Rails member
jeremy added a note May 8, 2012

Better to simply not add the deprecation to master

Removed deprecation from master, added PR #6221 for 3-2-stable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@alexeymuranov alexeymuranov commented on the diff Jan 1, 2012
activerecord/lib/active_record/fixtures.rb
end
- def yaml_fixtures_key(path)

not used.

@jeremy
Ruby on Rails member
jeremy added a note May 7, 2012

What's the git history on it? Just a forgotten method?

This method was created in the initial commit by dhh on 2004-11-23.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@arunagw
Ruby on Rails member

your PR needs a rebase!

Thanks :-)

@alexeymuranov

Voilà !

@arunagw
Ruby on Rails member

thanks for rebasing. Let me get someone to review this :-)

Cheers,
Arun

cc/ @spastorino can you look this PR ?

@rafaelfranca
Ruby on Rails member

@jeremy please take a look in this pull request and also see if this comment is still relevant

@jeremy jeremy and 1 other commented on an outdated diff May 7, 2012
activerecord/lib/active_record/fixtures.rb
@@ -371,20 +371,23 @@ class FixtureClassNotFound < ActiveRecord::ActiveRecordError #:nodoc:
# *DEFAULTS
#
# Any fixture labeled "DEFAULTS" is safely ignored.
- class Fixtures
+ class Fixtures # An instance of Fixtures can be called fixture_set, it is normally stored in a single YAML file and possibly in a folder with the same name.
+ # TODO: how about renaming Fixtures to FixtureSet (or FixtureS)?
+ # This would distinguish it nicely from TestFixtures module,
+ # where fixtures from multiple sets are kept together.
@jeremy
Ruby on Rails member
jeremy added a note May 7, 2012

Possible to rename, but it's public API, so you'd need to deprecate the old constant.

I would prefer to do it in a separate pull request, i think.

@jeremy
Ruby on Rails member
jeremy added a note May 8, 2012

+1. Can remove these comments, then.

done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jeremy jeremy and 1 other commented on an outdated diff May 7, 2012
activerecord/lib/active_record/fixtures.rb
@table_name = ( model_class.respond_to?(:table_name) ?
model_class.table_name :
- self.class.default_fixture_table_name(fixture_name) )
+ self.class.default_fixture_table_name(self.name) )
@jeremy
Ruby on Rails member
jeremy added a note May 7, 2012

Don't need self. qualifier

Removed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jeremy jeremy commented on the diff May 7, 2012
activerecord/lib/active_record/fixtures.rb
@@ -768,33 +762,39 @@ def try_to_load_dependency(file_name)
end
end
- def require_fixture_classes(fixture_names = nil)
- (fixture_names || fixture_table_names).each do |fixture_name|
- file_name = fixture_name.to_s
+ def require_fixture_classes(fixture_set_names = nil)
+ if fixture_set_names
+ fixture_set_names = fixture_set_names.map { |n| n.to_s }
+ else
+ fixture_set_names = fixture_table_names
@jeremy
Ruby on Rails member
jeremy added a note May 7, 2012

Why this special case? There's no guarantee that fixture_table_names is all strings

I was trying to avoid "defensive programming". If i understood correctly the rules about using strings and symbols, there is a consensus that for actual table names one should use strings anyway, isn't this so?

@jeremy
Ruby on Rails member
jeremy added a note May 7, 2012

But you can set fixture_table_names to symbols already. Then it breaks.

Ok, i can change it. I just thought that the recommended style was to always use strings for actual SQL table names, then the conversion would be redundant.

@jeremy, excuse me, are you sure it makes sense to allow non-strings for fixture_table_names? This class attribute is not documented and internally it only gets strings. Rails tests were passing with this revision.

(I remember there was even a discussion in #1363 whether to allow symbols for class names in declaring associations, while to me constant names are much more symbols than strings.)

@jeremy
Ruby on Rails member
jeremy added a note May 8, 2012

Table names are commonly provided as symbols with set_fixture_class. They're converted to strings at https://github.com/rails/rails/pull/4254/files#L0R439

I'd err on the side of being defensive here, in case an app or plugin is relying on the API.

(The original code makes it clearer that fixture_table_names acts as a default value, too.)

@jeremy, as a part of the changes that i've made, the hash keys for fixture classes are no longer expected to be 'table_names' but 'fixture/names', and they are stored as strings (see set_fixture_class).

fixture_table_names were not the actual table names, they were names provided when calling set_fixture_class or fixtures. They were parsed to guess the class name or the file name. I have made them convert to strings on input, so the only way to get a non-string in fixture_table_names is to call fixture_table_names= with non-strings directly, but this is not a documented API. This is why i think that the defensiveness here is not well justified :).

By the way, i propose to deprecate fixture_table_names in favor of fixture_set_names (another time).

@jeremy
Ruby on Rails member
jeremy added a note May 8, 2012
the hash keys for fixture classes are no longer expected to be 'table_names' but 'fixture/names', and they are stored as strings

Right, but set_fixture_class /accepts/ symbol keys.

In any case, I agree with you. Let's leave the change and see whether it breaks anything :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jeremy jeremy and 1 other commented on an outdated diff May 8, 2012
activerecord/lib/active_record/fixtures.rb
@@ -371,20 +371,20 @@ class FixtureClassNotFound < ActiveRecord::ActiveRecordError #:nodoc:
# *DEFAULTS
#
# Any fixture labeled "DEFAULTS" is safely ignored.
- class Fixtures
+ class Fixtures # An instance of Fixtures can be called fixture_set, it is normally stored in a single YAML file and possibly in a folder with the same name.
@jeremy
Ruby on Rails member
jeremy added a note May 8, 2012

Feels odd having a comment above this class and another one next to it. Could you include this in the main commentary?

I meant this as a comment for developers, not to show in Rdocs. Maybe i can move it down into the class body with a "NOTE:", what do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jeremy jeremy and 1 other commented on an outdated diff May 8, 2012
activerecord/lib/active_record/fixtures.rb
+ # Call as:
+ #
+ # instantiate_fixtures(object, fixture_set, load_instances = true)
+ def self.instantiate_fixtures(object, fixture_set_name, fixture_set = true,
+ load_instances = true)
+ if fixture_set == true || fixture_set == false
+ load_instances = fixture_set
+ fixture_set = fixture_set_name
+ else
+ ActiveSupport::Deprecation.warn(
+ "ActiveRecord::Fixtures.instantiate_fixtures with parameters"\
+ " (object, fixture_set_name, fixture_set, load_instances = true)"\
+ " is deprecated and may be removed from future releases,"\
+ " use it with parameters"\
+ " (object, fixture_set, load_instances = true) instead"\
+ " (skip fixture_set_name).", caller)
@jeremy
Ruby on Rails member
jeremy added a note May 8, 2012

Good place to use heredoc syntax.

A::D.warn <<-WARNING, caller
...
WARNING

What about line breaks, or should i format the text to a fixed width? Also, does it look ok to type everything flushed left?

I've searched a bit through the rails source: heredocs seem to mostly be used for code and marked up text like HTML. One of the exceptions where it is used for plain text message is this: https://github.com/rails/rails/blob/master/railties/lib/rails/ruby_version_check.rb
but i find it weird that the indent of the message would depend on the indent of the code where the message is defined. I also would not like to show off with a nicely formatted deprecation message among other deprecation messages :).

P.S. BTW, heredocs are weird ;).

P.P.S. If you insist, i can .strip_heredoc in PR to 3-2-stable, but they are still weird...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jeremy jeremy commented on an outdated diff May 8, 2012
activerecord/lib/active_record/fixtures.rb
@@ -411,11 +411,16 @@ def self.cache_fixtures(connection, fixtures_map)
cache_for_connection(connection).update(fixtures_map)
end
- def self.instantiate_fixtures(object, fixture_name, fixtures, load_instances = true)
+ #--
+ # TODO: in the next version, remove __with_new_arity suffix when
+ # the method with the old arity is completely removed.
+ #++
+ def self.instantiate_fixtures__with_new_arity(object, fixture_set,
+ load_instances = true) # :nodoc:
@jeremy
Ruby on Rails member
jeremy added a note May 8, 2012

Prefer not to wrap the method args like this, ok to use a longer line

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jeremy jeremy commented on an outdated diff May 8, 2012
activerecord/lib/active_record/fixtures.rb
@@ -423,62 +428,81 @@ def self.instantiate_fixtures(object, fixture_name, fixtures, load_instances = t
end
end
+ # The arity is deprecated: +fixture_set_name+ parameter is not used.
+ # Call as:
+ #
+ # instantiate_fixtures(object, fixture_set, load_instances = true)
+ def self.instantiate_fixtures(object, fixture_set_name, fixture_set = true,
+ load_instances = true)
@jeremy
Ruby on Rails member
jeremy added a note May 8, 2012

Ditto re. line wrap

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@alexeymuranov alexeymuranov and 1 other commented on an outdated diff May 8, 2012
activerecord/lib/active_record/fixtures.rb
+ #
+ # instantiate_fixtures(object, fixture_set, load_instances = true)
+ def self.instantiate_fixtures(object, fixture_set, load_instances = true, rails_3_2_compatibility_argument = true)
+ unless load_instances == true || load_instances == false
+ ActiveSupport::Deprecation.warn(
+ "ActiveRecord::Fixtures.instantiate_fixtures with parameters"\
+ " (object, fixture_set_name, fixture_set, load_instances = true)"\
+ " is deprecated and shall be removed from future releases. "\
+ " Use it with parameters"\
+ " (object, fixture_set, load_instances = true) instead"\
+ " (skip fixture_set_name).", caller)
+ fixture_set = load_instances
+ load_instances = rails_3_2_compatibility_argument
+ end
+ instantiate_fixtures__with_new_arity(object, fixture_set, load_instances)
+ end

Here is a better way to deprecate until removal, i think.

@jeremy
Ruby on Rails member
jeremy added a note May 8, 2012

Nice improvement

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
alexeymuranov added some commits Dec 31, 2011
@alexeymuranov alexeymuranov Remove unused parameter in ::instantiate_fixtures
Second parameter of Fixtures::instantiate_fixtures was not used, so i removed it.
af44c2b
@alexeymuranov alexeymuranov Remove unused private method in fixtures.rb
Remove the unused private method ActiveRecord::Fixtures#yaml_fixtures_key(path).
caeca1f
@alexeymuranov alexeymuranov Rename some variables
Rename some parameters and instance and local variables, mostly in fixtures.rb.  Also remove an unused assignment to an instance variable.

There are minor code changes.
12ceb45
@jeremy
Ruby on Rails member

Ok, looking good to me! Thanks for your patience, @alexeymuranov :)

@jeremy jeremy merged commit 42a14c4 into rails:master May 10, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment