Skip to content

Extract ActionView to separate directory #11032

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

Merged
merged 21 commits into from
Jun 20, 2013
Merged

Conversation

lukaszx0
Copy link
Member

Hey,

This is continuation of work done by @drogus. Basically it moves ActionView outside of ActionPack to its own directory. AV still remains dependency on AP, I will be working on this in coming weeks.

However, after discussing it with @drogus, both of us agreed that it would be good to have those commits in master. (they are basically just moving files, not much changes is code itself) because:

  • @kaspth is working on his GSoC Loofah project - it touches ActionView so it will be super hard to solve conflicts either for him or me once one of our branches get merged.
  • It'll be hard for me if somebody will make some changes in master to rebase it.

/cc @pixeltrix @drogus @kaspth

@kaspth
Copy link
Contributor

kaspth commented Jun 20, 2013

Sounds good to me.

@rafaelfranca
Copy link
Member

:shipit:

@lukaszx0
Copy link
Member Author

Cool. Let me fix few small issues from travis.

@@ -0,0 +1,3 @@
## Rails 4.1.0 (unreleased) ##
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should not be removed since all changelog files don't use it anymore?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, and I think we have to move some entries from the actionpack changelog to here.

@rafaelfranca
Copy link
Member

An entry on ActionPack CHANGELOG is good too. Maybe we have to review some documentation.

module ActionPack
# Returns the version of the currently loaded ActionView as a Gem::Version
def self.version
Gem::Version.new "4.0.0.beta1"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be "4.1.0.beta" no? (or at least "4.0.0.rc1")

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

4.1.0.beta

@robin850
Copy link
Member

Hi,

Very nice pull request! ❤️ Is it intended not to put a new line at the end of some files? If it's not the case, here are the concerned files:

  • actionview/test/fixtures/digestor/messages/show.html.erb
  • actionview/test/fixtures/fun/games/hello_world.erb
  • actionview/test/fixtures/good_customers/_good_customer.html.erb
  • actionview/test/fixtures/shared.html.erb
  • actionview/test/fixtures/test/_changing_priority.html.erb
  • actionview/test/fixtures/test/_changing_priority.json.erb
  • actionview/test/fixtures/test/_counter.html.erb
  • actionview/test/fixtures/test/_customer.erb
  • actionview/test/fixtures/test/_customer_greeting.erb
  • actionview/test/fixtures/test/_customer_with_var.erb
  • actionview/test/fixtures/test/_first_json_partial.json.erb
  • actionview/test/fixtures/test/_partial_only.erb
  • actionview/test/fixtures/test/_raise_indentation.html.erb
  • actionview/test/fixtures/test/_second_json_partial.json.erb
  • actionview/test/fixtures/test/hello.builder
  • actionview/test/fixtures/test/hello/hello.erb
  • actionview/test/fixtures/test/hello_world.erb

If it's intented, sorry for the spam!

Have a nice day.

@lukaszx0
Copy link
Member Author

Thanks @robin850 and @rafaelfranca for review. I've added few commits above addressing your comments and small things that I've found myself.

Regarding the new line at the end of the file - those files are copied as they were before, in actionpack, nothing's been changed there. I guess that adding new line at the end may break some tests? I don't know. Anyway - it's a matter of style and I don't have opinion on that. It's up to core team.

@guilleiguaran
Copy link
Member

Looks great for me 👍

@spastorino can be interested in review this also

@drogus
Copy link
Member

drogus commented Jun 20, 2013

@rafaelfranca any comments on new lines? I don't care to be honest, so if this is not a concern I will merge. I'm pretty sure that there can be things that need improvement here (like: tests are a bit bloated, AV tests using controllers etc), but I would like to merge it soon to not create extra work for @strzalek, like rebasing this branch every few days. We can iterate later.

@carlosantoniodasilva
Copy link
Member

Changing new lines in those files will likely break tests that check against their output, not expecting the new line at the end. As long as the files were untouched now, I think those changes can be done separately if necessary.

I'm 👍 on moving, this will avoid a lot of possible headache in future work as you guys have described.

@rafaelfranca
Copy link
Member

No problems with new lines :shipit:

@guilleiguaran
Copy link
Member

👍 please don't care about new lines

@rafaelfranca
Copy link
Member

Well, I think these files should not be removed actionpack/test/fixtures/test/malformed/malformed.html.erb~ and all the ~ files

@drogus
Copy link
Member

drogus commented Jun 20, 2013

Ok, tests are green, so I'm merging :D

@drogus
Copy link
Member

drogus commented Jun 20, 2013

@rafaelfranca ah, good catch!

@guilleiguaran
Copy link
Member

good catch @rafaelfranca, files with ~ are kept intentionally

@robin850
Copy link
Member

Sorry for the spam with new lines. ^^

2013/6/20 Guillermo Iguaran notifications@github.com

good catch @rafaelfranca https://github.com/rafaelfranca, files with ~are kept intentionally


Reply to this email directly or view it on GitHubhttps://github.com//pull/11032#issuecomment-19778448
.

lukaszx0 added 2 commits June 20, 2013 23:35
Now if somebody by mistake will remove malformed files test will raise error.
@lukaszx0
Copy link
Member Author

I've restored files and added assert to check if they exist. Next time if @drogus will remove them, he'll be punished by travis ;)

🚢

@@ -376,6 +376,7 @@ def test_render_knows_about_types_registered_when_extensions_are_checked_earlier
def test_render_ignores_templates_with_malformed_template_handlers
ActiveSupport::Deprecation.silence do
%w(malformed malformed.erb malformed.html.erb malformed.en.html.erb).each do |name|
assert File.exists?(File.expand_path("#{FIXTURE_LOAD_PATH}/test/malformed/#{name}~")), "Malformed file (#{name}~) which should be ignored does not exists"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

some people had tried to send PRs removing the ~ files, I think this makes clear that the files are intentionally kept

@drogus
Copy link
Member

drogus commented Jun 20, 2013

Next time if @drogus will remove them, he'll be punished by travis ;)

@drogus
Copy link
Member

drogus commented Jun 20, 2013

TravisCI approves 😎

drogus added a commit that referenced this pull request Jun 20, 2013
Extract ActionView to separate directory
@drogus drogus merged commit a29f746 into rails:master Jun 20, 2013
@lukaszx0
Copy link
Member Author

star-trek---183260

@spastorino
Copy link
Contributor

👍

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

Successfully merging this pull request may close these issues.

10 participants