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

Add suspenders:development:environment generator #1149

Merged

Conversation

crackofdusk
Copy link
Contributor

@crackofdusk crackofdusk commented Dec 11, 2023

Creates generator to configure the development environment. Keeps parity
with Rails as much as possible in an effort to avoid drift with Rails
standards.

It's possible a future release of Rails will alleviate us from having to
do the following:

  • enable active_model.i18n_customize_full_message which is being
    addressed in #50406
  • enable active_record.query_log_tags_enabled which is being addressed
    in #51342

@crackofdusk crackofdusk mentioned this pull request Dec 11, 2023
17 tasks
@crackofdusk crackofdusk force-pushed the suspenders-3-0-0-set-up-development-env branch from eab6dbe to 54d9428 Compare December 12, 2023 10:37
@stevepolitodesign
Copy link
Contributor

@crackofdusk

Should the email configuration be part of an suspenders:email_generator

I feel like it makes sense to create a suspenders:email_generator since that touches all environments.

Rails comes with a setup script. Do we feel strongly enough about our own setup script to override the default one?

I think this should be its own separate generator, since there's an opportunity to improve it. That would also be the place to handle dev:prime if we chose to.

Quiet assets is the default development configuration setting already. Do we care about configuring it for every environment? Can we assume that one is using a CDN in production anyway?

It looks like that was introduced in 2015 in 3a06edf and might be in reference to this gem. I think we should stick with the Rails defaults and not modify application.rb

What should the generator configuration look like? The customisations are only necessary when using rspec.

I don't think we need to configure the generators. If we still feel strongly about this, that could be a responsibility of the TestingGenerator (which does not yet exist).

suspenders:lint assumes the app uses yarn. Is that on purpose? Should we ship a bin/yarn script that makes sure yarn is installed?

Yes, this is on purpose. I don't think it's explicitly documented anywhere, but Rails seems to default to yarn. For example, if you install cssbundling-rails, it will install the dependencies with yarn. Additionally, Rails ships with a yarn Rake task

Copy link
Contributor

@stevepolitodesign stevepolitodesign left a comment

Choose a reason for hiding this comment

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

I love how lean this is! I think we should take this opportunity to review all configuration options.

For example, I think there's value in enabling config.action_view.annotate_rendered_view_with_filenames.

Do we want to enable config.active_record.strict_loading_by_default?

Comment on lines 8 to 10
def raise_on_missing_translations_in_test
uncomment_lines("config/environments/test.rb", "config.i18n.raise_on_missing_translations = true")
end
Copy link
Contributor

Choose a reason for hiding this comment

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

I recognize this is an open question in the pull request, but want to note that we're modifying files in the test environment even though this generator is called DevelopmentEnvironmentGenerator.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We might want to add this and other tweaks to the test environment, so I will remove this from the development environment generator.

@mike-burns
Copy link
Contributor

config.action_view.annotate_rendered_view_with_filenames

Into it.

config.active_record.strict_loading_by_default

Is this a development tool? This seems like something that will change semantics, and I wouldn't want it to be different in different environments.

@stevepolitodesign
Copy link
Contributor

@mike-burns

Is this a development tool? This seems like something that will change semantics, and I wouldn't want it to be different in different environments.

Yes. We have it enabled on in development and test on a client project. It's a reasonable substitute for bullet. However, it's challenging to work with when queries are made outside the application, like in gems. We've run into this with devise for example. Since we don't have access to the current_user definition, we need to redefine it and call includes.

@mike-burns
Copy link
Contributor

strict_loading_by_default

Oh I see how it works now. Wow that thing is under-documented.

Yup that makes sense to me.

@mike-burns
Copy link
Contributor

In Suspenders 2, we had namespaced generators such as suspenders:production:timeout and suspenders:staging:pull_requests. Do we like that pattern? Should this be suspenders:development:environment?

Copy link
Contributor

Choose a reason for hiding this comment

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

From a test-driven development perspective, the change to set config.action_dispatch.show_exceptions = :rescuable by default (introduced in rails/rails#45867 had had a largely negative impact on the feedback from Controller and System level test failures.

I'm advocating that Suspenders set config.action_dispatch.show_exceptions = :none so that tests fails with callstack information instead of HTML error pages.

@stevepolitodesign
Copy link
Contributor

stevepolitodesign commented Dec 19, 2023

Based on a recent client feature, I think there's value in enabling config.active_model.i18n_customize_full_message.

I've opened rails/rails#50406 to make this the default.

@crackofdusk crackofdusk changed the title Add suspenders:development_environment generator Add suspenders:development:environment generator Dec 22, 2023
@crackofdusk crackofdusk force-pushed the suspenders-3-0-0-set-up-development-env branch 3 times, most recently from 32e96e7 to e17e574 Compare December 22, 2023 13:03
@stevepolitodesign

This comment was marked as resolved.

@seanpdoyle

This comment was marked as resolved.

@stevepolitodesign

This comment was marked as resolved.

@stevepolitodesign
Copy link
Contributor

Noting we might want to enable config.active_record.query_log_tags_enabled = true.

https://github.com/rails/rails/pull/51342/files

stevepolitodesign pushed a commit that referenced this pull request Apr 5, 2024
Creates generator to configuration the production environment. For now,
this means [requiring a master key][master key].

Drops configuration for [asset_host][], since that should be an
infrastructure decision.

In an effort to distinguish between the development environment
configuration in #1149, we use the `production` namespace.

[master key]: https://guides.rubyonrails.org/configuring.html#config-require-master-key
[asset_host]: https://guides.rubyonrails.org/configuring.html#config-asset-host
@stevepolitodesign stevepolitodesign force-pushed the suspenders-3-0-0-set-up-development-env branch from e17e574 to 1711f5b Compare April 5, 2024 11:23
@stevepolitodesign stevepolitodesign marked this pull request as ready for review April 5, 2024 11:23
@stevepolitodesign stevepolitodesign self-assigned this Apr 5, 2024
stevepolitodesign pushed a commit that referenced this pull request Apr 5, 2024
Creates generator to configuration the production environment. For now,
this means [requiring a master key][master key].

Drops configuration for [asset_host][], since that should be an
infrastructure decision.

In an effort to distinguish between the development environment
configuration in #1149, we use the `production` namespace.

[master key]: https://guides.rubyonrails.org/configuring.html#config-require-master-key
[asset_host]: https://guides.rubyonrails.org/configuring.html#config-asset-host
stevepolitodesign added a commit that referenced this pull request Apr 5, 2024
Configures test environment. This differs from #1156 in that this commit
is concerned with configuration, where that commit was concerned with
generating a holistic test suite. It's also possible to run each
generator independently, and the two should not rely on one another.

Disables [action_dispatch.show_exceptions][] in an effort to [improve
failure output][comment].

Enables [raise_on_missing_translations][] to keep parity with the same
setting in development #1149

[action_dispatch.show_exceptions]: https://edgeguides.rubyonrails.org/configuring.html#config-action-dispatch-show-exceptions
[comment]: #1149 (comment)
[raise_on_missing_translations]: https://guides.rubyonrails.org/configuring.html#config-i18n-raise-on-missing-translations
stevepolitodesign pushed a commit that referenced this pull request Apr 5, 2024
Creates generator to configuration the production environment. For now,
this means [requiring a master key][master key].

Drops configuration for [asset_host][], since that should be an
infrastructure decision.

In an effort to distinguish between the development environment
configuration in #1149, we use the `production` namespace.

[master key]: https://guides.rubyonrails.org/configuring.html#config-require-master-key
[asset_host]: https://guides.rubyonrails.org/configuring.html#config-asset-host
stevepolitodesign added a commit that referenced this pull request Apr 5, 2024
Configures test environment. This differs from #1156 in that this commit
is concerned with configuration, where that commit was concerned with
generating a holistic test suite. It's also possible to run each
generator independently, and the two should not rely on one another.

Disables [action_dispatch.show_exceptions][] in an effort to [improve
failure output][comment].

Enables [raise_on_missing_translations][] to keep parity with the same
setting in development #1149

[action_dispatch.show_exceptions]: https://edgeguides.rubyonrails.org/configuring.html#config-action-dispatch-show-exceptions
[comment]: #1149 (comment)
[raise_on_missing_translations]: https://guides.rubyonrails.org/configuring.html#config-i18n-raise-on-missing-translations
@stevepolitodesign stevepolitodesign force-pushed the suspenders-3-0-0-set-up-development-env branch 2 times, most recently from 96180f8 to 12ed7db Compare April 5, 2024 14:20
@stevepolitodesign stevepolitodesign force-pushed the suspenders-3-0-0-set-up-development-env branch from 12ed7db to 4b8894d Compare April 5, 2024 17:30
stevepolitodesign added a commit that referenced this pull request Apr 5, 2024
Creates generator to configuration the production environment. For now,
this means [requiring a master key][master key].

Drops configuration for [asset_host][], since that should be an
infrastructure decision.

In an effort to distinguish between the development environment
configuration in #1149, we use the `production` namespace.

[master key]: https://guides.rubyonrails.org/configuring.html#config-require-master-key
[asset_host]: https://guides.rubyonrails.org/configuring.html#config-asset-host

Co-authored-by: Steve Polito <[email protected]>
stevepolitodesign added a commit that referenced this pull request Apr 5, 2024
Configures test environment. This differs from #1156 in that this commit
is concerned with configuration, where that commit was concerned with
generating a holistic test suite. It's also possible to run each
generator independently, and the two should not rely on one another.

Disables [action_dispatch.show_exceptions][] in an effort to [improve
failure output][comment].

Enables [raise_on_missing_translations][] to keep parity with the same
setting in development #1149

[action_dispatch.show_exceptions]: https://edgeguides.rubyonrails.org/configuring.html#config-action-dispatch-show-exceptions
[comment]: #1149 (comment)
[raise_on_missing_translations]: https://guides.rubyonrails.org/configuring.html#config-i18n-raise-on-missing-translations
stevepolitodesign added a commit that referenced this pull request Apr 5, 2024
Configures test environment. This differs from #1156 in that this commit
is concerned with configuration, where that commit was concerned with
generating a holistic test suite. It's also possible to run each
generator independently, and the two should not rely on one another.

Disables [action_dispatch.show_exceptions][] in an effort to [improve
failure output][comment].

Enables [raise_on_missing_translations][] to keep parity with the same
setting in development #1149

[action_dispatch.show_exceptions]: https://edgeguides.rubyonrails.org/configuring.html#config-action-dispatch-show-exceptions
[comment]: #1149 (comment)
[raise_on_missing_translations]: https://guides.rubyonrails.org/configuring.html#config-i18n-raise-on-missing-translations
@stevepolitodesign stevepolitodesign force-pushed the suspenders-3-0-0-set-up-development-env branch from 4b8894d to 50f2a8d Compare April 5, 2024 17:39
Creates generator to configure the development environment. Keeps parity
with Rails as much as possible in an effort to avoid drift with Rails
standards.

It's possible a future release of Rails will alleviate us from having to
do the following:

- enable `active_model.i18n_customize_full_message` which is being
  addressed in [#50406][]
- enable `active_record.query_log_tags_enabled` which is being addressed
  in [#51342][]

[#50406]: rails/rails#50406
[#51342]: rails/rails#51342
@stevepolitodesign stevepolitodesign force-pushed the suspenders-3-0-0-set-up-development-env branch from 50f2a8d to cca73b5 Compare April 5, 2024 17:39
@stevepolitodesign stevepolitodesign merged commit 7d666ca into suspenders-3-0-0 Apr 5, 2024
2 checks passed
@stevepolitodesign stevepolitodesign deleted the suspenders-3-0-0-set-up-development-env branch April 5, 2024 17:44
stevepolitodesign added a commit that referenced this pull request Apr 5, 2024
Follow-up to #1151, #1182, and #1149

Using the `Environments` namespace feels more appropriate, since this
keeps parity with Rails' directory structure.
stevepolitodesign added a commit that referenced this pull request Apr 5, 2024
Follow-up to #1151, #1182, and #1149

Using the `Environments` namespace feels more appropriate, since this
keeps parity with Rails' directory structure.
stevepolitodesign added a commit that referenced this pull request May 10, 2024
Creates generator to configuration the production environment. For now,
this means [requiring a master key][master key].

Drops configuration for [asset_host][], since that should be an
infrastructure decision.

In an effort to distinguish between the development environment
configuration in #1149, we use the `production` namespace.

[master key]: https://guides.rubyonrails.org/configuring.html#config-require-master-key
[asset_host]: https://guides.rubyonrails.org/configuring.html#config-asset-host

Co-authored-by: Steve Polito <[email protected]>
stevepolitodesign added a commit that referenced this pull request May 10, 2024
Configures test environment. This differs from #1156 in that this commit
is concerned with configuration, where that commit was concerned with
generating a holistic test suite. It's also possible to run each
generator independently, and the two should not rely on one another.

Disables [action_dispatch.show_exceptions][] in an effort to [improve
failure output][comment].

Enables [raise_on_missing_translations][] to keep parity with the same
setting in development #1149

[action_dispatch.show_exceptions]: https://edgeguides.rubyonrails.org/configuring.html#config-action-dispatch-show-exceptions
[comment]: #1149 (comment)
[raise_on_missing_translations]: https://guides.rubyonrails.org/configuring.html#config-i18n-raise-on-missing-translations
stevepolitodesign added a commit that referenced this pull request May 10, 2024
Creates generator to configure the development environment. Keeps parity
with Rails as much as possible in an effort to avoid drift with Rails
standards.

It's possible a future release of Rails will alleviate us from having to
do the following:

- enable `active_model.i18n_customize_full_message` which is being
  addressed in [#50406][]
- enable `active_record.query_log_tags_enabled` which is being addressed
  in [#51342][]

[#50406]: rails/rails#50406
[#51342]: rails/rails#51342

Co-authored-by: Steve Polito <[email protected]>
stevepolitodesign added a commit that referenced this pull request May 10, 2024
Follow-up to #1151, #1182, and #1149

Using the `Environments` namespace feels more appropriate, since this
keeps parity with Rails' directory structure.
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.

4 participants