Rails Ruby on Rails

Comments and changes to this ticket

• 

  Cezary Baginski March 28th, 2009 @ 01:20 PM

  • Tag changed from “2.3.2, action_view, loading, template, tests” to “2.3.2, action_view, loading, patch, template, tests”

• You flagged this item as spam.
  

  thedarkone March 28th, 2009 @ 04:34 PM

  I'd suggest you simply put item.html.erb.orig somewhere into template fixtures dir so you don't have to do all this brittle stubing...

  *.html.erb~ were handled correctly cause we managed to fix them before 2.3-final came around :).

• 

  Cezary Baginski March 28th, 2009 @ 11:12 PM

  Done. New patch replaces old one.

  You're right about the stubs being brittle.

  Yet I do tend to favor stubs in similar situations, especially when ruby classes are used (File, Time, Dir, etc) and when refactoring affected classes isn't first priority.

  Anyway, hope this is ok now. Thanks.

  • additional_template_tests_with_fixture.diff 3.1 KB

• You flagged this item as spam.
  

  thedarkone March 29th, 2009 @ 12:17 AM

  Hey, it's me again, sorry for being such a pain in the ass, but:

  • why do you care about grabbing, memoized ivar instead of just calling filename?

  • you don't need to manually call load! on ReloadablePath

  • you should remove the commented code, nobody is probably going to care about what exactly was broken in v2.3.2 tag in a few months

  • you can merge it all into one test method with something like

  
  [ActionView::EagerPath, ActionView::ReloadablePath].each ...
  

  • if you do merge it, you don't need to use constants

  • I'm not sure you need to test both ReloadablePath and EagerPath since both classes simply rely on accessible_paths returning correct paths

• 

  Cezary Baginski March 29th, 2009 @ 12:38 PM

  No problem - the pleasure is mine.

  You are right about everything. Rewrote the whole thing from scratch, split test into two (extension test + render test).

  I have doubts about what the extension/format extracting results should be for the files - I added a test that passes with the current implementation.

  It should be fine as long as no two filenames produce the same key for the path classes.

  Regarding your comments:

  memoized values - the filename method was added, which i missed while moving the tests from the 2.3.2 version. I mistakenly assumed it wouldn't work.

  load! - I missed the explicit load! in the [](path) method for EagerPath. Why didn't I actually check? Beats me.

  commented code - since the new version works (because of recent fixes in the handling of format/extensions), I needed a way to check if the tests actually detect the problem in the old version. Back then, I didn't know that filename worked...

  merging - yes, that is what I should have done.

  testing both classes - for me, working out why my *.orig files were loaded instead was very challenging. Especially since my development and test environments used different caching settings. The thing is if something changes in the future and template classes do not guarantee unique keys for every file in the view path, the easiest place to detect this would be from the path classes - and both implementations are currently share less code than they should, IMHO.

  Thanks for your detailed remarks and your time (honestly). I'll try not to be as sloppy in the future, although this issue has eaten up a lot more time than it was probably worth.

  I feel a lot better about the current patch. I'm eager to get this done properly, ignoring the time it will take me at this point, so any remarks are more than welcome.

  Thanks again.

  • addional_render_test_for_backup_files.diff 3.2 KB
• 

  Cezary Baginski March 29th, 2009 @ 12:40 PM

  Sorry about the load of text and the formatting.

• You flagged this item as spam.
  

  thedarkone March 29th, 2009 @ 03:32 PM

  • Assigned user set to “josh”

  The patch looks a lot better now.

  Templates don't guarantee returning unique paths, for example item.html.haml and item.html.erb will both rightfully claim to be reachable through item.html. The current API is slightly messed up and is completely rewritten in 3.0.

  You can also move your render test to test/template/render_test.rb if you are worried about differences between EagerPath and ReloadablePath.

  As for Eager and Reloadable classes not sharing enough code, you can move register_template into Path, but that's about it IMHO, fill free to submit a patch if you think otherwise :).

• 

  Cezary Baginski March 29th, 2009 @ 04:46 PM

  • Assigned user cleared.

  Good idea. I moved the test to template/render_test.rb - since they actually have nothing to do with the controller, after the :template option becomes :file anyway.

  The test runs twice and fails twice (eager/reloadable) on 2.3.2 as it should.

  I'm rather afraid that my lack of knowledge about template handling boundary cases and specifics will break more stuff than it will "fix".

  All the comments have been very helpful, thanks.

  • backup_file_rendering_tests.diff 2.6 KB
• 

  Cezary Baginski March 29th, 2009 @ 04:49 PM

  • Assigned user set to “josh”

  Oops. Cleared the field...

• 

  josh April 2nd, 2009 @ 01:32 AM

  • Milestone cleared.
  • State changed from “new” to “open”

  Looks good. I'll try to get it in soon.

• 

  Repository April 2nd, 2009 @ 06:05 PM

  • State changed from “open” to “resolved”

  (from [44423126c6f6133a1d9cf1d0832b527e8711d40f]) Additional template render test for backup files [#2367 state:resolved]

  Signed-off-by: Joshua Peek josh@joshpeek.com http://github.com/rails/rails/co...