I am fixing bugs in fixtures.rb, and would like to propose some API
and internal code changes.
1. Make TestFixtures::fixture_table_names hold a hash?
I would like to make the (undocumented?) class attribute
`TestFixtures::fixture_table_names` to hold a Hash of the form
```
{ "fixture_name" => :table_name, ... }
```
similarly to `TestFixtures::fixture_class_names` holding a Hash of the
form
```
{ "fixture_name" => FixtureModelClass, ... }
```
Currently `TestFixtures::fixture_table_names` holds an Array, which
does not seem very useful, seems inconsistent with
`fixture_class_names`, and is not apparently used anywhere except for
tests.
2. Models vs classes in variable and method names?
I would also propose to rename `fixture_class_names` to
`fixture_model_names` and `set_fixture_classes` to
`set_fixture_models`: this seems more flexible and descriptive to me.
3. Make set_fixture_classes only accept classes?
Currently both are acceptable:
```
set_fixture_classes "admin/users" => Administrator,
:customers => "Client"
```
Is there a situation where one cannot pass the class and can only pass
its name?
If `set_fixture_classes` would only accept classes, the code would be
somewhat simpler, i guess.
4. Standardize the fixture name in the form "admin/users" (rather
than :admin_users) and use it as fixtures''s identifier, instead of
table name?
It seems that originally the idea was to use the table name as the
unique identifier of the collections of fixtures intended to fill that
table, but now this seems all messed up, variable names like
`fixture_name`, `table_name`, `fixture_path` are used with different
contents and sometimes interchangeably.
Since the method `fixtures` accept their names in the form `"admin/
users"`, i propose to use this short path to the fixture file as the
name and identifier of the collection of fixtures, and use
`fixture_name` only in this sense.
The table name should be defined in the associated model, or by a new
method `set_fixture_tables` analogous to `set_fixture_classes`, and
only when there is no associated model, or the associated model does
not respond to `table_name` method and the table name was not set with
`set_fixture_tables`, should the table name be inferred from fixture
name by substitution `tr(''/'',''_'')`.
5. Rename Fixture to FixtureInstance and Fixtures to
FixtureCollection?
It took me some time to understand that `ActiveRecord::Fixtures` is
used for the contents of fixture files, and `ActiveRecord::Fixture` is
for a single record.
How about renaming `Fixture` to `FixtureInstance`?
I do not think this class is even documented, so shouldn''t be a
problem.
Also, maybe ''FixtureCollection'' would be better than
`Fixtures`?
It is also not clear to me why `ActiveRecord::Fixture` is needed:
isn''t it enough that each entry of `ActiveRecord::Fixtures` can be
converted to a hash or to an instance of the associated model?
What do you think?
Alexey.
--
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com.
To unsubscribe from this group, send email to
rubyonrails-core+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en.
On Dec 28, 1:27 am, Alexey <alexey.mura...@gmail.com> wrote:> I am fixing bugs in fixtures.rb, and would like to propose some API > and internal code changes. > > 1. Make TestFixtures::fixture_table_names hold a hash? > > I would like to make the (undocumented?) class attribute > `TestFixtures::fixture_table_names` to hold a Hash of the form > > ``` > { "fixture_name" => :table_name, ... } > ``` > > similarly to `TestFixtures::fixture_class_names` holding a Hash of the > form > > ``` > { "fixture_name" => FixtureModelClass, ... } > ``` > > Currently `TestFixtures::fixture_table_names` holds an Array, which > does not seem very useful, seems inconsistent with > `fixture_class_names`, and is not apparently used anywhere except for > tests. > > 2. Models vs classes in variable and method names? > > I would also propose to rename `fixture_class_names` to > `fixture_model_names` and `set_fixture_classes` to > `set_fixture_models`: this seems more flexible and descriptive to me. > > 3. Make set_fixture_classes only accept classes? > > Currently both are acceptable: > > ``` > set_fixture_classes "admin/users" => Administrator, > :customers => "Client" > ``` > > Is there a situation where one cannot pass the class and can only pass > its name? > If `set_fixture_classes` would only accept classes, the code would be > somewhat simpler, i guess. > > 4. Standardize the fixture name in the form "admin/users" (rather > than :admin_users) and use it as fixtures''s identifier, instead of > table name? > > It seems that originally the idea was to use the table name as the > unique identifier of the collections of fixtures intended to fill that > table, but now this seems all messed up, variable names like > `fixture_name`, `table_name`, `fixture_path` are used with different > contents and sometimes interchangeably. > Since the method `fixtures` accept their names in the form `"admin/ > users"`, i propose to use this short path to the fixture file as the > name and identifier of the collection of fixtures, and use > `fixture_name` only in this sense. > The table name should be defined in the associated model, or by a new > method `set_fixture_tables` analogous to `set_fixture_classes`, and > only when there is no associated model, or the associated model does > not respond to `table_name` method and the table name was not set with > `set_fixture_tables`, should the table name be inferred from fixture > name by substitution `tr(''/'',''_'')`. > > 5. Rename Fixture to FixtureInstance and Fixtures to > FixtureCollection? > > It took me some time to understand that `ActiveRecord::Fixtures` is > used for the contents of fixture files, and `ActiveRecord::Fixture` is > for a single record. > How about renaming `Fixture` to `FixtureInstance`? > I do not think this class is even documented, so shouldn''t be a > problem. > Also, maybe ''FixtureCollection'' would be better than `Fixtures`? > > It is also not clear to me why `ActiveRecord::Fixture` is needed: > isn''t it enough that each entry of `ActiveRecord::Fixtures` can be > converted to a hash or to an instance of the associated model? > > What do you think? > > Alexey.I forgot one more: 6. Rename ActiveRecord::TestFixtures.fixture_path to ActiveRecord::TestFixtures.fixtures_directory? `ActiveRecord::TestFixtures.fixture_path` holds the path to the fixture files directory, and is sometimes stored in `fixtures_directory` variable elsewhere (in `ActiveRecord::Fixtures.create_fixtures`), and `fixture_path` is often used for the path to a single fixture file. There seem also to be confusion with the use `fixture_path` and `fixtures_directory` local variables. A. -- You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com. To unsubscribe from this group, send email to rubyonrails-core+unsubscribe@googlegroups.com. For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en.
On Dec 28, 1:27 am, Alexey <alexey.mura...@gmail.com> wrote:> I am fixing bugs in fixtures.rb, and would like to propose some API > and internal code changes. > > 4. Standardize the fixture name in the form "admin/users" (rather > than :admin_users) and use it as fixtures''s identifier, instead of > table name? > > It seems that originally the idea was to use the table name as the > unique identifier of the collections of fixtures intended to fill that > table, but now this seems all messed up, variable names like > `fixture_name`, `table_name`, `fixture_path` are used with different > contents and sometimes interchangeably. > Since the method `fixtures` accept their names in the form `"admin/ > users"`, i propose to use this short path to the fixture file as the > name and identifier of the collection of fixtures, and use > `fixture_name` only in this sense. > The table name should be defined in the associated model, or by a new > method `set_fixture_tables` analogous to `set_fixture_classes`, and > only when there is no associated model, or the associated model does > not respond to `table_name` method and the table name was not set with > `set_fixture_tables`, should the table name be inferred from fixture > name by substitution `tr(''/'',''_'')`. >Just to illustrate what i am talking about, here is a line from fixtures.rb (around #807): ``` raise StandardError, "No fixture with name ''#{fixture}'' found for table ''#{fixture_name}''" ``` Alexey. -- You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com. To unsubscribe from this group, send email to rubyonrails-core+unsubscribe@googlegroups.com. For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en.
I''ve created a pull request with proposed changes, please let me know what you think: https://github.com/rails/rails/pull/4367 Alexey. -- You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com. To unsubscribe from this group, send email to rubyonrails-core+unsubscribe@googlegroups.com. For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en.