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 rails.png. #10056

Merged
merged 2 commits into from Apr 2, 2013
Merged

Remove rails.png. #10056

merged 2 commits into from Apr 2, 2013

Conversation

steveklabnik
Copy link
Member

No reason to have a file, we've got data-URIs now! It didn't make sense
to me that you no longer have to remove public/index.html, but you still
need to remove rails.png. This means that you won't have to any more
when making a new Rails app.

Now, a bunch of Railties tests fail because we use the fact that we know that rails.png exists to test them. I wanted to submit this as a PR to get feedback about the idea before I spent the time figuring out how to deal with that.

/cc @dhh

No reason to have a file, we've got data-URIs now! It didn't make sense
to me that you no longer have to remove public/index.html, but you still
need to remove rails.png. This means that you won't have to any more
when making a new Rails app.
@pablof7z
Copy link

pablof7z commented Apr 2, 2013

I celebrate most times a file is deleted or code removed from rails/rails. This is no exception. 👍

@chrismccord
Copy link

👍

1 similar comment
@assimovt
Copy link
Contributor

assimovt commented Apr 2, 2013

👍

@qrush
Copy link
Contributor

qrush commented Apr 2, 2013

👎, I think this is good for at least "Where do I put images in my app?"

@ejholmes
Copy link

ejholmes commented Apr 2, 2013

👍

@amiel
Copy link

amiel commented Apr 2, 2013

👍 It's a file that tends to stagnate in many projects. I'm always deleting it, but usually 3 months after the project was created because it was forgotten about.

And even if it's remembered early, it's still taking up space in git history.

@josevalim
Copy link
Contributor

@qrush I think it is ok to delete the file as long as we add a .gitkeep to the app/assets/images directory.

@buddhamagnet
Copy link
Contributor

I actually have a small script that removes the index.html and rails.png files. I tutted when I had to use it again today. This is good.

@reinh
Copy link

reinh commented Apr 2, 2013

👍

1 similar comment
@gregmolnar
Copy link
Member

👍

@chrismccord
Copy link

@qrush I think a .gitkeep in app/assets/images should suffice

@aviflombaum
Copy link

👍 ya, no reason to precompile it either.

@rafaelfranca
Copy link
Member

👍 with the .gitkeep

@mhenrixon
Copy link

👍

@heimidal
Copy link
Contributor

heimidal commented Apr 2, 2013

👍 I always forget that I need to delete it, and have wondered why it wasn't being handled with data-URI since index.html disappeared.

@mdespuits
Copy link

👍 Yes please! please! please!

@DevL
Copy link
Contributor

DevL commented Apr 2, 2013

👍

1 similar comment
@bai
Copy link

bai commented Apr 2, 2013

👍

dhh added a commit that referenced this pull request Apr 2, 2013
@dhh dhh merged commit 50fd48e into rails:master Apr 2, 2013
@steveklabnik
Copy link
Member Author

🤘

I'll commit test fixes soon. Working on them right now...

@frodsan
Copy link
Contributor

frodsan commented Apr 2, 2013

Does it need a CHANGELOG entry btw?

@steveklabnik
Copy link
Member Author

Good call. fc11375

@dhh
Copy link
Member

dhh commented Apr 3, 2013

IMO no need for a changelog entry. Nobody is going to care. Doesn't change anything for anyone in terms of functionality.

On Apr 2, 2013, at 17:38, Steve Klabnik notifications@github.com wrote:

Good call. fc11375


Reply to this email directly or view it on GitHub.

@carlosantoniodasilva
Copy link
Member

I can just think of a few templates trying to remove rails.png that might fail and will need to be changed now.

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.

None yet