Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Let users undo the administrate:views generator #424

Merged
merged 1 commit into from
Jan 22, 2016

Conversation

c-lliope
Copy link
Contributor

Fixes #310

Problem:

Rails provides a way to undo the effects of generators,
by running the command rails destroy GENERATOR_NAME.

With the way that we had set up our hierarchy of generators,
Rails did not know how to undo the effects of sub-generators.

For example, with the administrate:views generator,
we were calling Rails::Generators.invoke("administrate:views:index").

Whether the generator was run with the generate or destroy command
did not make a difference -
the index generator would always be invoked
as if it were run with generate.

Solution:

Rails generators use the @behavior instance variable to keep track of
how the generator was run.
This variable can be either :invoke or :revoke.

Pass this variable as an option to the sub-generators
to ensure they have the same behavior as the parent.

Minor changes:

Extract Administrate::GeneratorHelpers to store common
generator-related methods.

module Administrate
module GeneratorHelpers
def call_generator(generator, *args)
Rails::Generators.invoke(generator, args, behavior: behavior)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about passing behavior to call_generator? Otherwise, it seems like a mystery guest.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

normally I'd agree, but that would defeat the purpose of extracting this method. I wanted to automatically include the parent's behavior, instead of passing it in explicitly.

@gylaz
Copy link

gylaz commented Jan 21, 2016

Left a few suggestions/questions. Looking good!

run_generator [resource], behavior: :revoke

expect(Rails::Generators).
to invoke_generator("administrate:views:index", [resource], behavior: :revoke)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line is too long. [86/80]

Fixes #310

Problem:

Rails provides a way to undo the effects of generators,
by running the command `rails destroy GENERATOR_NAME`.

With the way that we had set up our hierarchy of generators,
Rails did not know how to undo the effects of sub-generators.

For example, with the `administrate:views` generator,
we were calling `Rails::Generators.invoke("administrate:views:index")`.

Whether the generator was run with the `generate` or `destroy` command
did not make a difference -
the index generator would always be invoked
as if it were run with `generate`.

Solution:

Rails generators use the `@behavior` instance variable to keep track of
how the generator was run.
This variable can be either `:invoke` or `:revoke`.

Pass this variable as an option to the sub-generators
to ensure they have the same behavior as the parent.

Minor changes:

Extract `Administrate::GeneratorHelpers` to store common
generator-related methods.
@c-lliope c-lliope force-pushed the gw-destroy-generators branch from 1b7a29f to 1146204 Compare January 22, 2016 06:53
@c-lliope c-lliope closed this Jan 22, 2016
@c-lliope c-lliope merged commit 1146204 into master Jan 22, 2016
@c-lliope c-lliope deleted the gw-destroy-generators branch January 22, 2016 06:55
c-lliope added a commit that referenced this pull request Jan 22, 2016
Changes:

```
* [#269] [FEATURE] Add a generator for copying default layout files
* [#328] [FEATURE] Add a generator for copying default sidebar partial
* [#362] [FEATURE] Add a generator for only the dashboard manifest.
  Customizing this manifest before running the `administrate:install` generator
  will change which dashboards get generated.
* [#295] [FEATURE] Add dashboard detection for ActiveRecord::Enum fields.
* [#364] [FEATURE] Improve dashboard generator by explicitly listing out the
  generated `SHOW_PAGE_ATTRIBUTES` array elements.
* [#416] [UI] Add an accessibility label to the search input
* [#411] [UI] Use tabular figures in table cells
* [#409] [UI] Use default system fonts
* [#424] [BUGFIX] Fix a bug where running `rails destroy GENERATOR_NAME`
  would not work for several of the generators
* [#390] [BUGFIX] Fix timestamp deprecation warnings
* [#365] [COMPAT] Remove dependency on `inline_svg`
* [#396] [I18n] Ukrainian
* [#297] [I18n] Italian
* [#307] [I18n] Fix German grammatical errors
* [#363] [DOC] Move documentation into main repository, at the root URL
* [#395] [DOC] Update inline documentation for collection partial
* [#387] [DOC] Fix incorrect path for generators in the docs
```
@c-lliope c-lliope mentioned this pull request Jan 22, 2016
c-lliope added a commit that referenced this pull request Jan 22, 2016
Changes:

```
* [#269] [FEATURE] Add a generator for copying default layout files
* [#328] [FEATURE] Add a generator for copying default sidebar partial
* [#362] [FEATURE] Add a generator for only the dashboard manifest.
  Customizing this manifest before running the `administrate:install` generator
  will change which dashboards get generated.
* [#295] [FEATURE] Add dashboard detection for ActiveRecord::Enum fields.
* [#364] [FEATURE] Improve dashboard generator by explicitly listing out the
  generated `SHOW_PAGE_ATTRIBUTES` array elements.
* [#416] [UI] Add an accessibility label to the search input
* [#411] [UI] Use tabular figures in table cells
* [#409] [UI] Use default system fonts
* [#424] [BUGFIX] Fix a bug where running `rails destroy GENERATOR_NAME`
  would not work for several of the generators
* [#390] [BUGFIX] Fix timestamp deprecation warnings
* [#365] [COMPAT] Remove dependency on `inline_svg`
* [#396] [I18n] Ukrainian
* [#297] [I18n] Italian
* [#307] [I18n] Fix German grammatical errors
* [#363] [DOC] Move documentation into main repository, at the root URL
* [#395] [DOC] Update inline documentation for collection partial
* [#387] [DOC] Fix incorrect path for generators in the docs
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants