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

Collapse all new default initializers into a single file #25231

Merged
merged 1 commit into from Jun 1, 2016

Conversation

Projects
None yet
6 participants
@prathamesh-sonpatki
Member

prathamesh-sonpatki commented Jun 1, 2016

  • Adjusted tests also for this new behavior.
  • Based on the discussion in
    #25184 (comment).

r? @dhh

Collapse all new default initializers into a single file
- Adjusted tests also for this new behavior.
- Based on the discussion in
  #25184 (comment).
@prathamesh-sonpatki

This comment has been minimized.

Member

prathamesh-sonpatki commented Jun 1, 2016

Build failures are unrelated.

@nthj

This comment has been minimized.

Contributor

nthj commented Jun 1, 2016

I love this. It cleans up the initializers folder, and intuitively I think it encourages new developers to familiarize themselves with the available configurations. Thanks, @prathamesh-sonpatki. :)

@maclover7

This comment has been minimized.

Member

maclover7 commented Jun 1, 2016

Do we need to write up some internal docs about how changing framework defaults will work going forward?

@dhh

This comment has been minimized.

Member

dhh commented Jun 1, 2016

@maclover7 I think this is more self-explanatory than what we had before. Before it was a grey area of whether the initializers were intended to be tweaked or whether they were new framework defaults. Now the file is literally called new_framework_defaults.rb. So think we're good.

@dhh dhh merged commit 1221eb4 into rails:master Jun 1, 2016

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
@kaspth

This comment has been minimized.

Member

kaspth commented Jun 1, 2016

Love this, great simplification! I got some post review though :)

so it is introduced as a configuration option to ensure that apps
made on earlier versions of Rails are not affected when upgrading.

This is mentioned in every config. We should move this to the top of the file, so users know what to expect within it.

Though for every config it would be nice to see what the 4.2 version of the config was and the new 5.0 default + maybe a sentence about why it's changing. Showing past and present defaults would help knowing what to set the flags to and how far you are in staying up to date.

kaspth added a commit that referenced this pull request Jun 1, 2016

Merge pull request #25231 from prathamesh-sonpatki/collapse-new-initi…
…alizers

Collapse all new default initializers into a single file
@kaspth

This comment has been minimized.

Member

kaspth commented Jun 1, 2016

@dhh should this be backported to 5.0.0? It would make sense because that's when people are most likely to run bin/rails app:update and get the file.

Backported to 5-0-stable @ f79dec5.

@dhh

This comment has been minimized.

Member

dhh commented Jun 1, 2016

Yes please!

On Wed, Jun 1, 2016 at 11:54 AM, Kasper Timm Hansen <
notifications@github.com> wrote:

@dhh https://github.com/dhh should this be backported to 5.0.0? It
would make sense because that's when people are most likely to run bin/rails
app:update and get the file.

Backported to 5-0-stable @ f79dec5
f79dec5
.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#25231 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAAKtSpy1vP8P65ut8W18qo1s0Bnk42Nks5qHVbzgaJpZM4IrFhk
.

@prathamesh-sonpatki prathamesh-sonpatki deleted the prathamesh-sonpatki:collapse-new-initializers branch Jun 1, 2016

@prathamesh-sonpatki

This comment has been minimized.

Member

prathamesh-sonpatki commented Jun 1, 2016

@kaspth We don't have any Rails 4.2 alternatives for any of the config options here.

Except for per_form_csrf_tokens and forgery_protection_origin_check all of them require the notice - 'This is new Rails 5.0 default ... backward compatibility...`.

Opened #25235 with some more cleanup.

kaspth added a commit that referenced this pull request Jun 1, 2016

Merge pull request #25231 from prathamesh-sonpatki/collapse-new-initi…
…alizers

Collapse all new default initializers into a single file
@kaspth

This comment has been minimized.

Member

kaspth commented Jun 1, 2016

@dhh backported to 5-0-0 @ 5bcbd44

@prathamesh-sonpatki sure, but doesn't mean we couldn't say something like:

# Rails 5 makes `belongs_to` required by default while they were optional in past versions.
# This migration option, which will be removed in the next Rails release, you can opt-out
# with false.
Rails.application.config.active_record.belongs_to_required_by_default = true
@prathamesh-sonpatki

This comment has been minimized.

Member

prathamesh-sonpatki commented Jun 1, 2016

@kaspth Not sure about

# This migration option, which will be removed in the next Rails release, you can opt-out
# with false.

Do we want to which will be removed in the next Rails release ? Rest looks fine to me. 👍

@kaspth

This comment has been minimized.

Member

kaspth commented Jun 1, 2016

You're right. We could communicate that at the top of the file.

That's basically what I'm looking for, that the file sets expectations slightly better: these are migration options and you'd best start making your app use the new defaults as soon as you're on 5.0 (or whatever the release might be).

@prathamesh-sonpatki

This comment has been minimized.

Member

prathamesh-sonpatki commented Jun 1, 2016

Ok. I will incorporate this in #25235

prathamesh-sonpatki added a commit to prathamesh-sonpatki/rails that referenced this pull request Jun 4, 2016

More cleanup of new framework defaults
- Move real new default options to the top of the file.
- After that club together all the options which were added to keep
  backward compatibility. So all of them will get only one header.
- Based on rails#25231 (comment).
@djmaze

Sorry for the spam - Cannot delete this ;-/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment