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

Remove core JSON rendering #1063

Closed
Ortuna opened this issue Feb 20, 2013 · 7 comments
Closed

Remove core JSON rendering #1063

Ortuna opened this issue Feb 20, 2013 · 7 comments

Comments

@Ortuna
Copy link
Member

Ortuna commented Feb 20, 2013

Just to come full circle with the whole JSON rendering feature(from #1044). I believe the auto rendering to JSON in rendering.rb should be removed.

This should allow for:

  • Removing the expensive JSON gems(not sure if we can, but seems like we can. Nothing else is using it).
  • Leverage sinatra-contrib for any JSON rendering.

This does break 2 tests, and would be a breaking change.
Tests break here and here

What is proposed:

I can submit code for these changes but I'm not familiar with the procedures for "breaking" changes in Padrino.

Appreciate your time, thanks for the hard work on Padrino!

@ujifgc
Copy link
Member

ujifgc commented Feb 20, 2013

I think this is a good initiative. Json usage is something which should be stipulated on design phase of developing an application, not something you jump to use half way. Interface related gems should not hang there just in case. It might be needed for some UI-heavy gems like padrino-admin, but not padrino-core.

@achiurizo
Copy link
Member

👍 what do you guys think @padrino/core-members ?

@DAddYE
Copy link
Member

DAddYE commented Feb 20, 2013

Yea I agree!

@nesquena
Copy link
Member

Yes, I agree I don't think JSON needs to be baked. Breaking changes are OK but obviously we like to avoid them where we can. In this case, I don't think the negative fallout will be very high for removing this.

@Ortuna
Copy link
Member Author

Ortuna commented Feb 21, 2013

Thanks all, this was merged.

@Ortuna Ortuna closed this as completed Feb 21, 2013
@nesquena
Copy link
Member

Thank you. Appreciate the recent patches.

@postmodern
Copy link
Contributor

The downside of this, is that sinatra-contrib pulls in many other dependencies (namely eventmachine), just for a method as simple as:

def json(object)
  content_type :json
  object.to_json
end

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

No branches or pull requests

6 participants