Fix Issue #2572 with set_fixture_class by normalizing key format #3844
Conversation
| @@ -391,7 +391,7 @@ class Fixtures | |||
|
|
|||
| @@all_cached_fixtures = Hash.new { |h,k| h[k] = {} } | |||
|
|
|||
| def self.find_table_name(table_name) # :nodoc: | |||
There was a problem hiding this comment.
Good renaming such a name, I mean the new one has a better name clear it is just called here.
|
I am checking out your master to see if it works in my env. Regarding the migration I am looking into it. |
|
Thanks. Actually, my fixes are in my_fix_for_fixture_classes branch. I've noticed some other unclear behavior. I think there need to be methods for the conversions fixture/name <-> table_name <-> ClassName. Sometimes it is not clear whether a fixture or table name has to be a string, a symbol, or can be both. Also set_fixture_class :client => User, :organization => "Company"I wonder if this is not redundant in tests, and if it wouldn't be enough to only accept class constants, but this would break the current functionality, so it can only be considered for a separate patch. |
|
By the way, how about renaming |
| class_names[n.tr('/', '_').to_sym] = n.classify if n.include?('/') | ||
| } | ||
| def self.create_fixtures(fixtures_directory, table_names, fixture_classes = {}) | ||
| table_names = [table_names].flatten.map(&:to_s) |
There was a problem hiding this comment.
Can you please avoid changing code that doesn't relate to the functionality you want to add? It makes reviewing the diff too hard.
|
Hi, can you please squash these commits and make the diff as small as possible? There are many places where I can't tell if variable names are being changed or if functionality is being changed. Thanks. |
|
I am relatively new to GitHub, if i squash the commits, do i need to close this pull request and open a new one? I do not see how to squash commits inside existing pull request. |
|
You can squash them and push to the same branch giving a -f as option (to |
|
Tests are passing now, i think i've fixed another bug. |
| @@ -441,9 +441,6 @@ def self.instantiate_all_loaded_fixtures(object, load_instances = true) | |||
|
|
|||
| def self.create_fixtures(fixtures_directory, table_names, class_names = {}) | |||
| table_names = [table_names].flatten.map { |n| n.to_s } | |||
| table_names.each { |n| | |||
| class_names[n.tr('/', '_').to_sym] = n.classify if n.include?('/') | |||
| } | |||
There was a problem hiding this comment.
This was breaking set_fixture_class for "namespaced" fixtures.
Test using fixtures with random names and model names, that is not following naming conventions but using set_fixture_class instead. It is expected that the table name be defined in the model, but this is not explicitly tested here. This will need to be fixed.
This solves an issue with set_fixture_class class method caused by create_fixtures method's overwriting passed to it fixture model classes, when the fixtures are "namespased": foo/bar or admin/users.
The idea is to use "foo/bar" string as the name and identifier of a
fixture file bar in directory foo. The model class of the fixture is either set with set_fixture_class method, or otherwise inferred from its name using camelize method.
Also a bug is fixed in lines 487-489 when the table names were guessed by substitution from the fixture file names, ambiguously called table_names, instead of being taken from fixture attributes. Now they are taken from attributes.
I plan to submit another fix so that the fixture's table name (for
example foo_bar) be defined by the fixture's model whenever possible,
instead of inferring it from the fixture's name ("foo/bar").
"Normalize" how
set_fixture_classstores the keys: now for a namespaced fixture the fixture name should be passed and stored in the format"admin/users"(or:"admin/users") instead of:admin_usersor"admin_users". This is consistent with how fixture names are passed tofixturesmethod. Previously namespaced fixtures were treated separately in some cases which madeset_fixture_classuseless for namespaced fixtures. Overall, there was a lack of consistency in what was used as a fixture name and identifier:"foo/bar"or:foo_bar. This inconsistency is not completely resolved by this patch.This patch fixes Issue #2572:
set_fixture_classwith namespaced fixtures, like inhad no effect.