Json xml #96

Closed
wants to merge 39 commits into
from

Conversation

Projects
None yet
2 participants
@GerryG
Collaborator

GerryG commented Mar 11, 2013

The plan for this is to reduce it to the parts that must be in the core and more the rest to the appropriate external container (TBD, maybe a gem)

It is ATP now, and we can have the necessary debate about the right way to change routing.

Gerry Gleason added some commits Jan 13, 2013

- rendered_text = renderer.render_show view_opts
- render :text=>rendered_text, :status=> renderer.error_status || status
+ rendered_obj = renderer.render_show view_opts
+ render obj_sym => rendered_obj, :status => renderer.error_status || status

This comment has been minimized.

Show comment Hide comment
@GerryG

GerryG Mar 11, 2013

Collaborator

I think this change is common to action_events. I think it makes sense, but I know you had some questions before.

@GerryG

GerryG Mar 11, 2013

Collaborator

I think this change is common to action_events. I think it makes sense, but I know you had some questions before.

This comment has been minimized.

Show comment Hide comment
@ethn

ethn Mar 12, 2013

Collaborator

This means that when the JSON Renderer does its rendering, you get a ruby object? That won't fly. How would you ever attach JSON to an email? Any time we're dependent upon the controller to do work that we might want in controller-less contexts, it's a no-go imo.

@ethn

ethn Mar 12, 2013

Collaborator

This means that when the JSON Renderer does its rendering, you get a ruby object? That won't fly. How would you ever attach JSON to an email? Any time we're dependent upon the controller to do work that we might want in controller-less contexts, it's a no-go imo.

This comment has been minimized.

Show comment Hide comment
@ethn

ethn Mar 14, 2013

Collaborator

I also realized another case that this complicates. Suppose I have a card on wagn with json or xml in it. So it's already text. How would that work if the renderer isn't allowed to discriminate?

@ethn

ethn Mar 14, 2013

Collaborator

I also realized another case that this complicates. Suppose I have a card on wagn with json or xml in it. So it's already text. How would that work if the renderer isn't allowed to discriminate?

match 'recent(.:format)' => 'card#read', :id => '*recent', :view => 'content'
+
+ resources :card, :path => '/', :except => :show

This comment has been minimized.

Show comment Hide comment
@GerryG

GerryG Mar 11, 2013

Collaborator

I think your comment before was that we really only use a few of the routes created with this. With my last fixup, the card_controller handles the #index action too, so the match '/' isn't needed. YMMV

@GerryG

GerryG Mar 11, 2013

Collaborator

I think your comment before was that we really only use a few of the routes created with this. With my last fixup, the card_controller handles the #index action too, so the match '/' isn't needed. YMMV

@@ -460,7 +459,7 @@ def add_name_context name=nil
end
- class Renderer::JsonRenderer < Renderer
+ class Renderer::Json < Renderer

This comment has been minimized.

Show comment Hide comment
@GerryG

GerryG Mar 11, 2013

Collaborator

This can be promoted either way, we are eliminating the Renderer on Json and Html as a general pattern.

@GerryG

GerryG Mar 11, 2013

Collaborator

This can be promoted either way, we are eliminating the Renderer on Json and Html as a general pattern.

This comment has been minimized.

Show comment Hide comment
@ethn

ethn Apr 19, 2013

Collaborator

I don't think we can do this until we stop autoloading.

@ethn

ethn Apr 19, 2013

Collaborator

I don't think we can do this until we stop autoloading.

@@ -9,7 +9,6 @@ module Set::All::Kml
render( args[:view] || :search)
end
- # FIXME: integrate this with common XML features when it is added

This comment has been minimized.

Show comment Hide comment
@GerryG

GerryG Mar 11, 2013

Collaborator

Ok, I guess we should complet this on this branch now.

@GerryG

GerryG Mar 11, 2013

Collaborator

Ok, I guess we should complet this on this branch now.

@ethn

This comment has been minimized.

Show comment Hide comment
@ethn

ethn Mar 11, 2013

Collaborator

ticket?

Collaborator

ethn commented Mar 11, 2013

ticket?

@GerryG

This comment has been minimized.

Show comment Hide comment
@GerryG

GerryG Mar 11, 2013

Collaborator
Collaborator

GerryG commented Mar 11, 2013

@ethn

This comment has been minimized.

Show comment Hide comment
@ethn

ethn Mar 11, 2013

Collaborator

this could be trimmed down a lot. I don't think there's any way we want to maintain multiple copies of #wrap, #build_link, #get_layout_content, etc

Collaborator

ethn commented Mar 11, 2013

this could be trimmed down a lot. I don't think there's any way we want to maintain multiple copies of #wrap, #build_link, #get_layout_content, etc

@ethn

This comment has been minimized.

Show comment Hide comment
@ethn

ethn Apr 19, 2013

Collaborator

I'm going to close this PR, because I don't think there's much we're going to retain here. All the stuff in controllers is a major architectural violation. Almost all the stuff in renderers / specs is indiscriminate cut-and-paste. We should certainly keep it around, but this feels like a step backwards to me.

Collaborator

ethn commented Apr 19, 2013

I'm going to close this PR, because I don't think there's much we're going to retain here. All the stuff in controllers is a major architectural violation. Almost all the stuff in renderers / specs is indiscriminate cut-and-paste. We should certainly keep it around, but this feels like a step backwards to me.

@ethn ethn closed this Apr 19, 2013

@ethn

This comment has been minimized.

Show comment Hide comment
@ethn

ethn Apr 19, 2013

Collaborator

hmm, not all the stuff in controllers is violating (at least not the xml processing stuff). will continue to study...

Collaborator

ethn commented Apr 19, 2013

hmm, not all the stuff in controllers is violating (at least not the xml processing stuff). will continue to study...

@GerryG

This comment has been minimized.

Show comment Hide comment
@GerryG

GerryG May 4, 2013

Collaborator

Yeah, it is in the wrong place, but from an agile standpoint that should be ok until we have more functionality to merge and we can refactor so that this has a better home. As we have discussed, we need to spec all of it better and then implement to that.

Collaborator

GerryG commented May 4, 2013

Yeah, it is in the wrong place, but from an agile standpoint that should be ok until we have more functionality to merge and we can refactor so that this has a better home. As we have discussed, we need to spec all of it better and then implement to that.

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