Change fixtures API and internal code

39 views
Skip to first unread message

Alexey

unread,
Dec 27, 2011, 7:27:00 PM12/27/11
to Ruby on Rails: Core
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.

Alexey

unread,
Dec 28, 2011, 8:48:01 AM12/28/11
to Ruby on Rails: Core
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.

Alexey

unread,
Dec 29, 2011, 10:23:54 AM12/29/11
to Ruby on Rails: Core
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.

Alexey

unread,
Jan 6, 2012, 4:41:51 PM1/6/12
to Ruby on Rails: Core
I've created a pull request with proposed changes, please let me know
what you think:

https://github.com/rails/rails/pull/4367

Alexey.
Reply all
Reply to author
Forward
0 new messages