Skip to content

Added support to Ruby & VIM script languages#17

Closed
samflores wants to merge 2 commits intoPrismJS:gh-pagesfrom
samflores:gh-pages
Closed

Added support to Ruby & VIM script languages#17
samflores wants to merge 2 commits intoPrismJS:gh-pagesfrom
samflores:gh-pages

Conversation

@samflores
Copy link
Copy Markdown
Contributor

I added a quick support to the languages I use most.

@LeaVerou
Copy link
Copy Markdown
Member

LeaVerou commented Aug 1, 2012

Hi there,

Thank you so much for your effort and contribution!
However, it seems your editor changed the indentation of several files. Not only this breaks the code conventions of the project (tabs for indentation, spaces for alignment), but it also makes it incredibly hard to review the actual, "real" changes.

Would you be kind enough to fix this and submit a new pull request? :)

Thanks!!

@LeaVerou LeaVerou closed this Aug 1, 2012
@mrbrdo
Copy link
Copy Markdown

mrbrdo commented Sep 24, 2013

Would still be nice to merge this. Not merging because of a couple of spaces that a decent text editor can fix in seconds is a little ridiculous.

@apfelbox
Copy link
Copy Markdown
Contributor

@mrbrdo You can extract the content and resubmit it as new PR (if @samflores is ok with that)

@mrbrdo
Copy link
Copy Markdown

mrbrdo commented Sep 24, 2013

My god, I just finished rebasing and squashing these commits to finally realise you're using tabs for indentation. That's retarded. And then you jump on the guy that's using sane conventions. You don't even have newlines at the end of files. Sigh, huge disappoint.

@samflores
Copy link
Copy Markdown
Contributor Author

I actually made another PR (#18) using the project conventions. Turns out it wasn't accepted too and I don't have the time to fix it right now.

@LeaVerou
Copy link
Copy Markdown
Member

@mrbrdo: I’m sorry, but these are the conventions of the project. I have my reasons for using them but this is not the place for that debate. Also, if you’re disappointed in a project because of the indentation style and the lack of newlines at the end of files, I’ll happily live without your contributions. :)
I don’t get how you expect for a PR that changes the project’s conventions to be accepted. It’s pretty common knowledge in open source that project conventions are to be respected and followed. It’s also common knowledge in programming that good coders don’t throw temper tantrums over disagreement with said conventions. That’s why most editors have file-level settings.

That said, I could have reviewed this by adding ?w=1 to the URL, which I didn’t know a year ago. However, it has a couple other issues as well, which I explained in #18.

@samflores I’m sorry about that. I really want Ruby in Prism, so if I had the time to fix it myself, I would. :/ If you ever decide to work on it again, let me know if you need any help.

@mrbrdo
Copy link
Copy Markdown

mrbrdo commented Sep 26, 2013

I don't really care which conventions you use as far as whitespace goes, but if someone goes through a lot of work and is then turned down because of something like that, I don't consider it normal or nice at all. Considering it would not take you more than a few minutes to do it yourself, and since it is your decision to enforce this rules. A lot of text editors Ruby people use strip out trailing whitespace automatically and convert tabs to spaces too. In such a case it would be considerably easier and faster for you to do that than for someone with said editor.

@LeaVerou
Copy link
Copy Markdown
Member

@mrbrdo The reason I used to reject PRs because of whitespace changes is that without w=1, it was impossible to properly review the diff. Although, intentionally contributing to a project with your own preferences instead of following the code style of the project doesn’t exactly show good team spirit (in most cases it’s not intentional though).
Also, removing trailing whitespace is good, I never rejected anything because of that! It’s just that my editor doesn’t do it, so there’s always some left in there.

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.

4 participants