-
Notifications
You must be signed in to change notification settings - Fork 605
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
Clean up kernel platform #3199
Clean up kernel platform #3199
Conversation
Before actually merging this stuff in, we might want to discuss standards on syntax. The styleguide Rubinius provides is rather liberal. Did the Rubinius community consider using Rubocop (or Pronto) or commiting to the Ruby styleguide before? |
There are a few things worth noting here:
So tl;dr: Style wise, in general Rubinius doesn't strictly follow any "official" (read: decided by several members of the community) style guides other than the one defined on the Rubinius website. We certainly don't use any tools to enforce them either. The primary reason is that it doesn't really help us to have some tool bark at developers when they use an explicit |
In my opinion, many of these changes make the code uglier, which in turn illustrates the difficulty in coming to a consensus about rigid style rules. |
@yorickpeterse, I do agree with you on the parentheses. After submitting the PR it felt a bit dirty. I will remove them somewhere tomorrow. I do not agree on the double to single quotes. I did not make these changes with performance in mind (though I am aware of their benefit). I think they primarily show some intent. Here is a simple plain string, or here is a string in which something interesting is going to happen. @jemc, besides the parentheses (which I will gladly remove), what parts are you referring to? |
I don't have an opinion as to whether this makes the code uglier or prettier. But I would prefer not to see large commits that are just adjusting spacing and/or other minor formatting issues. It makes it harder to merge the mainline with the branch I'm working on, and the commit doesn't appear to have any functional purpose. |
@southerngs, you are definitely right. I assumed there was no active work on these files in other branches. (Most files where not touched in the last year.) Learned yet another thing, thanks. |
In the end I think it is better to just close this pull request. I do not want to complicate other work by just fixing indentation and some other minor issues. Thank you for your reflections, I learned something there on the code itself and collaboration. For the future I hope there will be more clear guidelines on style, but I am not sure how to address this topic properly and start the discussion in the community. |
David, We really appreciate the effort that you put in. Every little bit helps. I think you have already started the conversation about guidelines, so why not open an issue on this topic (and close this PR) and we can continue it there. We learned a few things. One, DSLs generally don’t have parentheses. If that isn’t in the Rubinius style guide then we have an obvious patch to make to the guide. Second, there is no clear guideline on where/when to use single versus double quotes. Again, if that isn’t in the style guide then we have a discussion point for a second patch. The style guide should be part of the rubinius docs which are in its own repository, so PRs can be opened against it just like for code. If you would like to discuss these issues in real-time, then you may consider joining the #rubinius channel at irc.freenode.net. It’s usually pretty active though not necessarily for all timezones. Thank you for your contribution to the community. Even though this first PR isn’t going to make it in, it’s a good bet that your second (and third and nth) will. Don’t give up! On Nov 12, 2014, at 4:07 AM, David Boot notifications@github.com wrote:
|
Am I right in understanding this PR can be closed? |
Yeah, and hopefully we can flesh out the style guide over in the docs. |
Cleaned up the syntax in
kernel/platform
. Changes focus on indentation, whitespace, single quoted strings, method invocation with parentheses,or
,and
andreturn
statements.