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

Drop PADRINO_ENV in favour of RACK_ENV #1445

Merged
merged 2 commits into from Jan 20, 2014

Conversation

Projects
None yet
8 participants
@dariocravero
Contributor

dariocravero commented Oct 10, 2013

Hey @padrino/core-members,

I've been thinking about this one for a while and 0.12 might be a good place to introduce the change. I can't really find a reason why we would like to have our own PADRINO_ENV variable when we could just reuse RACK_ENV and be more compatible with with Sinatra and Rack based apps.

In this PR I'm replacing all the occurrences of PADRINO_ENV for RACK_ENV.

Thoughts?

@ujifgc

This comment has been minimized.

Show comment
Hide comment
@ujifgc

ujifgc Oct 10, 2013

Member

👍 Good change!

Member

ujifgc commented Oct 10, 2013

👍 Good change!

@namusyaka

This comment has been minimized.

Show comment
Hide comment
@namusyaka

namusyaka Oct 10, 2013

Member

Great!

Member

namusyaka commented Oct 10, 2013

Great!

@skade

This comment has been minimized.

Show comment
Hide comment
@skade

skade Oct 10, 2013

Member

I think for compat reasons, we should still accept PADRINO_ENV, but prefer RACK_ENV if both are present and warn in that case.

Member

skade commented Oct 10, 2013

I think for compat reasons, we should still accept PADRINO_ENV, but prefer RACK_ENV if both are present and warn in that case.

@dariocravero

This comment has been minimized.

Show comment
Hide comment
@dariocravero

dariocravero Oct 10, 2013

Contributor

Yeah, I thought about that too @skade. In that case we can flag that PADRINO_ENV is being deprecated and that it will be gone in 1.0?

Contributor

dariocravero commented Oct 10, 2013

Yeah, I thought about that too @skade. In that case we can flag that PADRINO_ENV is being deprecated and that it will be gone in 1.0?

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Oct 10, 2013

Member

Yeah I agree, let's still support PADRINO_ENV for 0.12.0 (for compat reasons since it doesn't cost us much) but I like the direction towards just using RACK_ENV.

Member

nesquena commented Oct 10, 2013

Yeah I agree, let's still support PADRINO_ENV for 0.12.0 (for compat reasons since it doesn't cost us much) but I like the direction towards just using RACK_ENV.

@wikimatze

This comment has been minimized.

Show comment
Hide comment
@wikimatze

wikimatze Oct 16, 2013

Member

Jup, I'm for a deprecation warning - and finally get rid of this piece in Padrino 1.0!

Member

wikimatze commented Oct 16, 2013

Jup, I'm for a deprecation warning - and finally get rid of this piece in Padrino 1.0!

@ujifgc

This comment has been minimized.

Show comment
Hide comment
@ujifgc

ujifgc Oct 16, 2013

Member

Maybe we could use env['RACK_ENV'], but default it to env['PADRINO_ENV'] in cli base?

I think anywhere else it's just pointless. Do we run padrino tests in env other than test? Do we want new generated apps to contain deprecated environment var?

Member

ujifgc commented Oct 16, 2013

Maybe we could use env['RACK_ENV'], but default it to env['PADRINO_ENV'] in cli base?

I think anywhere else it's just pointless. Do we run padrino tests in env other than test? Do we want new generated apps to contain deprecated environment var?

@fj

This comment has been minimized.

Show comment
Hide comment
@fj

fj Dec 26, 2013

Contributor

+1 for this. Moving to RACK_ENV would also vastly increase compatibility with other third-party Rack services that expect to see RACK_ENV.

Contributor

fj commented Dec 26, 2013

+1 for this. Moving to RACK_ENV would also vastly increase compatibility with other third-party Rack services that expect to see RACK_ENV.

@fj

This comment has been minimized.

Show comment
Hide comment
@fj

fj Dec 26, 2013

Contributor

The main issue people seem to have is disliking the hard removal of PADRINO_ENV and that folks would rather see a deprecation-first approach. Either way I'd love to see Padrino switch to using RACK_ENV.

What if I made a new pull request (note: I'm not the original requester) so that it:

  • warns of deprecated usage in cli/base.rb if PADRINO_ENV is set
  • use RACK_ENV as the authoritative environment source in lib/padrino-core.rb
  • if PADRINO_ENV is set, sets RACK_ENV to PADRINO_ENV
  • if PADRINO_ENV is not set, sets RACK_ENV to options.environment

Would that be acceptable for inclusion?

Contributor

fj commented Dec 26, 2013

The main issue people seem to have is disliking the hard removal of PADRINO_ENV and that folks would rather see a deprecation-first approach. Either way I'd love to see Padrino switch to using RACK_ENV.

What if I made a new pull request (note: I'm not the original requester) so that it:

  • warns of deprecated usage in cli/base.rb if PADRINO_ENV is set
  • use RACK_ENV as the authoritative environment source in lib/padrino-core.rb
  • if PADRINO_ENV is set, sets RACK_ENV to PADRINO_ENV
  • if PADRINO_ENV is not set, sets RACK_ENV to options.environment

Would that be acceptable for inclusion?

@ujifgc

This comment has been minimized.

Show comment
Hide comment
@ujifgc

ujifgc Dec 26, 2013

Member

Yes.
On Dec 26, 2013 7:52 PM, "John Feminella" notifications@github.com wrote:

The main issue people seem to have is disliking the hard removal of
PADRINO_ENV. What if I made a new pull request (note: I'm not the original
requester) so that it:

  • warns of deprecated usage in cli/base.rb if PADRINO_ENV is set
  • use RACK_ENV as the authoritative environment source in
    lib/padrino-core.rb
  • if PADRINO_ENV is set, sets RACK_ENV to PADRINO_ENV
  • if PADRINO_ENV is not set, sets RACK_ENV to options.environment

Would that be acceptable for inclusion?


Reply to this email directly or view it on GitHubhttps://github.com/padrino/padrino-framework/pull/1445#issuecomment-31225058
.

Member

ujifgc commented Dec 26, 2013

Yes.
On Dec 26, 2013 7:52 PM, "John Feminella" notifications@github.com wrote:

The main issue people seem to have is disliking the hard removal of
PADRINO_ENV. What if I made a new pull request (note: I'm not the original
requester) so that it:

  • warns of deprecated usage in cli/base.rb if PADRINO_ENV is set
  • use RACK_ENV as the authoritative environment source in
    lib/padrino-core.rb
  • if PADRINO_ENV is set, sets RACK_ENV to PADRINO_ENV
  • if PADRINO_ENV is not set, sets RACK_ENV to options.environment

Would that be acceptable for inclusion?


Reply to this email directly or view it on GitHubhttps://github.com/padrino/padrino-framework/pull/1445#issuecomment-31225058
.

@fj

This comment has been minimized.

Show comment
Hide comment
@fj

fj Dec 26, 2013

Contributor

Great, I'll take a crack at this soon.

So that I'm being consistent with the rest of the codebase, are there existing examples of a preferred strategy for deprecating old Padrino stuff (e.g. calling some common deprecation method as Rails does)?

Contributor

fj commented Dec 26, 2013

Great, I'll take a crack at this soon.

So that I'm being consistent with the rest of the codebase, are there existing examples of a preferred strategy for deprecating old Padrino stuff (e.g. calling some common deprecation method as Rails does)?

@ujifgc

This comment has been minimized.

Show comment
Hide comment
@ujifgc

ujifgc Dec 26, 2013

Member

No, not really. Just warn about it.

Member

ujifgc commented Dec 26, 2013

No, not really. Just warn about it.

@skade

This comment has been minimized.

Show comment
Hide comment
@skade

skade Dec 26, 2013

Member

When switching the Rakefile format, we did not only warn, but also gave an explanation on how to migrate and why it is better.

Member

skade commented Dec 26, 2013

When switching the Rakefile format, we did not only warn, but also gave an explanation on how to migrate and why it is better.

@skade

This comment has been minimized.

Show comment
Hide comment
@skade

skade Dec 26, 2013

Member

Jep.

Member

skade commented Dec 26, 2013

Jep.

@Ortuna

This comment has been minimized.

Show comment
Hide comment
@Ortuna

Ortuna Dec 26, 2013

Member

I propose that we add a deprecated method along the lines of warn. This way a plugin can scan deprecations and we can log to the console a bit more distinctly from all warns. e.g.

  deprecated(:padrino_env, 'PADRINO_ENV has been deprecated in favor of RACK_ENV')
Member

Ortuna commented Dec 26, 2013

I propose that we add a deprecated method along the lines of warn. This way a plugin can scan deprecations and we can log to the console a bit more distinctly from all warns. e.g.

  deprecated(:padrino_env, 'PADRINO_ENV has been deprecated in favor of RACK_ENV')
@ujifgc

This comment has been minimized.

Show comment
Hide comment
@ujifgc

ujifgc Dec 26, 2013

Member

Well, as I said, just a warn scary enough to pay attention.

Member

ujifgc commented Dec 26, 2013

Well, as I said, just a warn scary enough to pay attention.

@ujifgc

This comment has been minimized.

Show comment
Hide comment
@ujifgc

ujifgc Dec 26, 2013

Member

@Ortuna I agree, this would organize things.

Member

ujifgc commented Dec 26, 2013

@Ortuna I agree, this would organize things.

@fj

This comment has been minimized.

Show comment
Hide comment
@fj

fj Dec 26, 2013

Contributor

Arguably the new deprecation can be done as a new PR; is that okay? I'd
rather not expand the scope here too much.
On Dec 26, 2013 1:22 PM, "Igor Bochkariov" notifications@github.com wrote:

@Ortuna https://github.com/Ortuna I agree, this would organize things.


Reply to this email directly or view it on GitHubhttps://github.com/padrino/padrino-framework/pull/1445#issuecomment-31230141
.

Contributor

fj commented Dec 26, 2013

Arguably the new deprecation can be done as a new PR; is that okay? I'd
rather not expand the scope here too much.
On Dec 26, 2013 1:22 PM, "Igor Bochkariov" notifications@github.com wrote:

@Ortuna https://github.com/Ortuna I agree, this would organize things.


Reply to this email directly or view it on GitHubhttps://github.com/padrino/padrino-framework/pull/1445#issuecomment-31230141
.

@ujifgc

This comment has been minimized.

Show comment
Hide comment
@ujifgc

ujifgc Dec 26, 2013

Member

Sure.

Member

ujifgc commented Dec 26, 2013

Sure.

@fj

This comment has been minimized.

Show comment
Hide comment
@fj

fj Dec 27, 2013

Contributor

I started working on this but ran into another bug which I fixed separately: #1533

I'll check back next week and give it another go. ❤️

Contributor

fj commented Dec 27, 2013

I started working on this but ran into another bug which I fixed separately: #1533

I'll check back next week and give it another go. ❤️

@nesquena nesquena referenced this pull request Jan 5, 2014

Closed

Release 0.12.0 #1019

ujifgc added a commit that referenced this pull request Jan 20, 2014

Merge pull request #1445 from padrino/use_rack_env
Deprecate PADRINO_ENV in favour of RACK_ENV

@ujifgc ujifgc merged commit 6cb2894 into master Jan 20, 2014

1 check was pending

default The Travis CI build is in progress
Details

@ujifgc ujifgc deleted the use_rack_env branch Jan 20, 2014

namusyaka added a commit to namusyaka/sample_blog that referenced this pull request May 25, 2014

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