rstacruz / sinatra-assetpack

Package your assets transparently in Sinatra.
http://ricostacruz.com/sinatra-assetpack/
MIT License
542 stars 97 forks source link

This allows for users to set the environment as a string #190

Closed dhananjaysathe closed 9 years ago

dhananjaysathe commented 9 years ago

This allows for users to set the environment as a string , often code uses string environment names and not symbols , this would wourk just as well with either without much effort

j15e commented 9 years ago

Are you sure this is a issue? Because it seems Sinatra is already forcing to_sym on settings.environment by default if you use ENV['RACK_ENV']. The documentation also suggest you use a symbol if you set it otherwise.

All in all, unfortunately I think this should not be dealt within sinatra-assetpack.

dhananjaysathe commented 9 years ago

Hey , So the convention of setting a symbol is recommended but at best it's a convention. The code you point out works in case an app does not explicitly set it. In cases where applications configure from an alternate data source and use environment in configuration this could very well be a string ( we render yml dynamically from an API call to a config server or from ETCD in a dockerized auto reconf deploy use case) Also one may use :deployment, :release etc too we choose to use :production for assetpack's sake. The addition of a .to_sym is a neat solution , if it's a symbol it will be unchanged if string "production" it'll still just work, one stands to not loose anything by having this in place. There is no "overhead" per-say , and it retains all compat it already has.It'll be good to have this upstream. My two cents.

j15e commented 9 years ago

I will merge since I see no bad side effect of doing it, but I would prefer to replace this whole configuration of output style with a setting like settings.assets.combine [true|false] rather than depend solely on the current sinatra env, which would allow to manage it like we want in any environment (staging, etc) & then you could do in your own app settings.assets.combine (settings.env.to_sym == 'production').

This was is something already discussed in #162 & #180.