sketchup-stl icon indicating copy to clipboard operation
sketchup-stl copied to clipboard

Ruby Style Agreement

Open briangbrown opened this issue 12 years ago • 24 comments

I would suggest an agreement on a Ruby style guide to follow. It doesn't really matter which one, just an agreed upon once to make style decisions clear and easy. Because we are developing on github and it looks reasonable may I suggest https://github.com/styleguide/ruby

briangbrown avatar Oct 26 '12 02:10 briangbrown

That works for me. Are we using the TomDoc style to document methods as well?

thomthom avatar Oct 26 '12 07:10 thomthom

Notes on the style guide:

Never use for, unless you know exactly why. Most of the time iterators should be used instead. for is implemented in terms of each (so you're adding a level of indirection), but with a twist - for doesn't introduce a new scope (unlike each) and variables defined in its block will be visible outside it.

I use for more often that each these days - mainly because it doesn't introduce a new scope. Because it doesn't introduce a new scope the variables inside the loop isn't created and destroyed for every iteration - as oppose to what each does. Creating variables are slow - and when you iterate entity collections where you have thousands to millions of items the performance can be noticed. So in terms of iterating large collections such as entities I do recommend for loops.

thomthom avatar Oct 26 '12 09:10 thomthom

And I just want to highlight this item on the style list:

The and and or keywords are banned. It's just not worth it. Always use && and || instead.

I see that many people actually use and and or - which I think is because it appear to be more natural in terms how you read it - without realizing that they function differently from && and ||. I've always used && and || because they are more predictable IMO.

thomthom avatar Oct 26 '12 09:10 thomthom

What about this one?

http://www.caliban.org/ruby/rubyguide.shtml#style

This is the one we're attempting to use internally.

scottlininger avatar Oct 26 '12 15:10 scottlininger

Works for me Scott.

briangbrown avatar Oct 27 '12 02:10 briangbrown

Seems to be quite similar to the other one. (The other one seemed to explain thing better IMO.) Works for me. Can we convert the importer to 2-space indentation now, please?

thomthom avatar Oct 29 '12 21:10 thomthom

I see Jim removes his feature branches after they've been merged. Is this normal to do? How does that affect the commit history?

thomthom avatar Oct 30 '12 11:10 thomthom

Another workflow question:

I've just completed the Merge coplanar faces feature - which I commited to a feature branch "feature-merge-faces" and made a Pull request.

What do I do if I want to work on some other thing until it's merged? I'd like to fix the path issue and some other small fixes. But how could I commit that? To a new branch based on the one I'd just worked on?

Or jump back to the master branch - without the merge option feature - and create a new parallel one?

thomthom avatar Oct 30 '12 11:10 thomthom

http://scottchacon.com/2011/08/31/github-flow.html

Read this article from the GitHub people - but still not wiser on how to work on multiple fixes while awaiting a Pull Request.

thomthom avatar Oct 30 '12 12:10 thomthom

Note for cleaning up:

Exporter is using quite a few and operators intead of &&.

Example:

if (foo == 'bar') and (biz == 'baz') and (bim == 'bam')

Which is more cleanly written as: (And recommended by all style guides I've seen)

if foo == 'bar' && biz == 'baz' && bim == 'bam'

thomthom avatar Oct 30 '12 13:10 thomthom

Yeah, I'm not sure the best way to handle these things. I just merged your first request, and the "new" pull request comes across nicely vs. the "latest & greatest" that is now in the repo. So I think this is a decent way to work.

scottlininger avatar Oct 30 '12 15:10 scottlininger

So what do I now do with my branches? Delete them, and pull the master branch from this main repo?

thomthom avatar Oct 30 '12 15:10 thomthom

Yeah, I seems silly that you'd have to keep old branches around. I suspect github will preserve all of our history for us. So delete away and let's see what happens.

scottlininger avatar Oct 30 '12 15:10 scottlininger

So, what is the preferred format for documenting methods? RDoc? YARD?

I personally prefer the structured and guided manner YARD document methods. RDoc is too freestyling and hippie-like IMO.

thomthom avatar Nov 01 '12 15:11 thomthom

@jimfoltz : what's your workflow inregard to: https://github.com/SketchUp/sketchup-stl/issues/11#issuecomment-9910516

thomthom avatar Nov 07 '12 10:11 thomthom

My workflow is still evolving.

I was trying to use branches to group together smaller, related commits for GitHub pull requests. I am not sure how this is working for others. Maybe since the files here are fairly small, it is easier to just review the entire file rather than review each tiny change.

I feel stuck at the moment because I am not sure what will happen with the current pull requests. I am not sure if I should base new changes on my branch which may or may not be accepted, or if I should always go ack to the current sketchup/aster branch and create parallel branches from there.

jimfoltz avatar Nov 08 '12 09:11 jimfoltz

What do you do with branches after they are merged with this repo's master? Delete? Erase? Merge with your master?

I also feel a bit stuck. You've got some large changes, but I want to wait until it's been reviewed and merged before I continue.

The SketchUppers are in Las Vegas I think, at Trimble Dimension event. Once Scott is back from that we can get your branch merged.

thomthom avatar Nov 08 '12 09:11 thomthom

Btw, what timezone are you on Jim..?

thomthom avatar Nov 08 '12 09:11 thomthom

US Eastern.

I haven't settled on a branch workflow, because I'm still unsure how best to use them on GitHub. But this page[1] and the next has a good explanation of branches. If you want to be able to easily go back to commit, keep the branch (or tag it.)

[1] http://think-like-a-git.net/sections/experimenting-with-git.html

jimfoltz avatar Nov 08 '12 09:11 jimfoltz

Hey guys, sorry I disappeared. We've indeed been in Vegas at the Trimble Dimensions conference. I'm reviewing requests now... :)

scottlininger avatar Nov 08 '12 20:11 scottlininger

Useful and interesting breakdown of some Git concepts: http://think-like-a-git.net/sections/testing-out-merges.html

thomthom avatar Nov 27 '12 12:11 thomthom

Don't think we reached a conclusion regarding what style to follow. Anyone?

thomthom avatar Nov 30 '12 13:11 thomthom

Re: Style Guides (1) The internal GitHub guide has no weight over other guides. Personally I find it inferior to others, and much of it very subjective to the author. It's only pro is the amount of reasons given (although I disagree with many of the reasons.)

(2) Most of the guides are written towards System Ruby scripts where there is no danger of clashing with other coder's scripts. The caliban.org guide is a case in point. They want coders to put include statements before class and module definitions. That is a no-no. That would mix in libraries into the toplevel ObjectSpace! The clause about wrapping a test in an if conditional that compares the current script path to $0 is another. (In SketchUp embedded Ruby the $0 global is the string "SketchUp".

(3) A shebang is not needed in a embedded Ruby code file, but the new magic # encoding: UTF-8 1st line comment should be in every file.

(4) I hate confusing "always do this, except in these cases", etc. Case in point whitespace. Either you always use it around = and operators, or you don't. The bottom line is Ruby is supposed to be human readable. That means whitespace. Making little exceptions like "not after (" or "not before )" do not make code more readable. It does the opposite. Personally with regard to parenthesis and square brackets (subscripts/indices) whatever makes it more readable. I like to put spaces around the argument, IF the expression is complex, or a constant or long-named variable reference. But if it is a literal number index or a small iterator variable like i, then I will not put spaces around it.

(5) The doc generator may not belong in a style guide. Every guide specifies some other format. Personally I have not found one I like. I think a method's comments & documentation should go inside itself. I fold up may scripts using Notepad++'s ALT+number (where the number is the nesting level to collapse. SHFT+ALT+number expands that nesting level. When I want the methods of a class or module collapsed, I meant it! I still do not want to be scrolling through a bunch of commenting. Some of these doc generators insist that the comments go just before the method definition. I put method notes just below in def line at the same indent. It looks like the comments are hanging from the definition statement. Same for modules and classes.

DanRathbun avatar Apr 27 '15 04:04 DanRathbun

We decided on the GitHub Ruby styleguide for the SketchUp GitHub project a good while ago. This issue just wasn't updated. Simply because it's a well known style that most open source involved people is familiar with. And consistency is the biggest value to a style guide.

Need to update the README to reflect this.

thomthom avatar Apr 27 '15 08:04 thomthom