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

Change README example tabs to ruby style 2 spaces #7

Merged
merged 1 commit into from
Apr 10, 2017

Conversation

picatz
Copy link
Member

@picatz picatz commented Apr 9, 2017

Love this project.

I'm going over some stuff to help you out. First thing that I noticed whilst going over the readme was the tabs in the example.

Kind of a small change, but I think it makes the example in the readme as displayed on GitHub much cleaner by using a ruby style 2 spaces for tabs.

@tarcieri
Copy link

tarcieri commented Apr 9, 2017

I would agree the style is a bit jarring as two space indentation is practically ubiquitous in Ruby. Perhaps it would be worth adopting rubocop like the other socketry projects, and trying to stick close to the defaults.

@ioquatix ioquatix merged commit 6184c29 into socketry:master Apr 10, 2017
@ioquatix
Copy link
Member

Thanks for this.

I'm okay with implementing a RubyCop policy as long as it doesn't objectively make the code worse. I did have a few issues with how timers was changed to suit RubyCop, things that I thought made the code less readable.

On tabs vs spaces, I don't find it jarring, but I do appreciate that the indentation is pretty deep. Perhaps I'm just used to reading all kinds of code that it doesn't bother me so much. Actually, after reading the ruby source code and editing it, nothing really phases me any more :D

I filed an issue with github (isaacs/github#170 (comment)), because they way the handle tabs is non-existant/poorly thought out. You can control the size of tabs using .editorconfig but it doesn't work for code in .md files. Additionally the whole point of tabs is to allow the user to set the width they prefer - e.g. a lot of Ruby developers prefer 2, but on my wider screens I like 4.

I invite everyone to discuss the issue here: #10

@tarcieri
Copy link

I did have a few issues with how timers was changed to suit RubyCop, things that I thought made the code less readable.

Simple rule of thumb for RuboCop: if a particular cop annoys you, disable it

@ioquatix
Copy link
Member

Hahah, with that logic, I might end up with an empty .rubocop eventually :D j/k

@picatz
Copy link
Member Author

picatz commented Apr 10, 2017

I'm actually working on doing some rubocop styling and inline yarddoc documentation for this gem. Because I like it; and I am learning more about it through the documentation process and modifying a few things in the process following rubocop as a guide to remove redundant returns, and non-breaking changes like that under the hood of some methods. I'm using the current specs to check that I'm not breaking things. Hopefully amiright?

Been doing so ever since I made this PR. I was sort of doing it on and off whilst working on other stuff.

You can check out that stuff in this forked branch thiny ma'jig if you'd like.

@ioquatix
Copy link
Member

On the case of the return statement. Please don't remove if it the function has more than 1 line of code. Even though it's redundant, in a code which isn't purely an accessor, I prefer that it's explicit. This isn't just some random choice that I make, I don't just write code and randomly decide whether or not to use return. It's an annotation to make it clearer that the return value is not just by accident, but an explicit part of the design.

@ioquatix
Copy link
Member

Also, I know that I'm going to stir up some frustrations, but converting all the code to use spaces for indentation is not a great PR right now, but it IS on the table for discussion. Let's focus on improving the documentation, and keep that separate from any PR addressing stylistic changes - in other words right now any changes of substance are appreciated but changes to style are going to be more contentious because they add nothing to the project and until we decide on a way forward, it's not something I even want to address at this time.

@ioquatix
Copy link
Member

Any PR that contributes improvements to documentation, fixes bugs, or otherwise fixes issues, is going to be merged almost right away :)

@picatz
Copy link
Member Author

picatz commented Apr 10, 2017

@ioquatix fo'shizzle -- I'll make sure whatever PRs for changes I make for the time being are purely documentation related.

#2-spaces4-life-squad-comen-8u-4real-2good-1life-0choices-whatamidoing

@picatz
Copy link
Member Author

picatz commented Apr 10, 2017

Gunna put it here since the issue was locked before I could add the comment:

My first thought when I wanted to contribute to stuff was "gotta change dem tabz"; and in general, in order to be more widely appealing, even at the contributor level, I figured it'd be something to think about.

And I'm glad we're doing that! 💃

I don't really think the 2-spaces argument is about more contributions, less bugs, easier to maintain completely; and I especially don't stand behind the argument that it's just what I personally prefer as being an acceptable reason.

Mah Reasonz

If we're speaking a language, and we want it to be best understood by those who speak that language, then it may be best ( in some situations ) to adopt the standards of that language in order to communicate the ideas in that language in the best possible way.

More Important

Now, I guess when it comes down to it, if it were to honestly make @ioquatix less excited to work on this stuff -- then I want no part in this campaign for 2-space justice any longer. Sure, I'd disagree with it a little. But, for real, if you really don't want the 2-spaces then I won't harass you about it anymore and I'll work around it.

Ruby

Becuase if it made you less happy, then it probably wouldn't be all that ruby like. Sure, it'd make me happy -- but I didn't really do anything.

With that said: #2-spaces4-life-squad-comen-8u-4real-2good-1life-0choices-whatamidoing

@ioquatix
Copy link
Member

You should be able to contribute to that issue as you are a contributor, is it not possible now?

@picatz
Copy link
Member Author

picatz commented Apr 10, 2017

nope

I tried to. 🤐

@picatz
Copy link
Member Author

picatz commented Apr 10, 2017

much lock very secure

much lockz

@ioquatix
Copy link
Member

Ah that's crappy :( I'm sorry for that. I've added you as a collaborator, and locked master branch, feel free to make PRs directly here, and then we can merge them.

@ioquatix
Copy link
Member

You should have got an invite by now via GitHub.

@picatz picatz deleted the fix-tabs-readme-example branch April 10, 2017 02:40
@picatz
Copy link
Member Author

picatz commented Apr 10, 2017

I got it. Thanks! 👍

@ioquatix
Copy link
Member

I'd love it if you could repost #7 (comment) on the relevant issue :)

@picatz
Copy link
Member Author

picatz commented Apr 10, 2017

Done dizzle ma' bizzle.

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

3 participants