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

A lot of improvements on formatting and syntax best practices #145

Merged
merged 3 commits into from
Dec 4, 2013

Conversation

ggarnier
Copy link
Contributor

@ggarnier ggarnier commented Dec 3, 2013

I used Rubocop to search for formatting and syntax warnings. There are still a lot of warnings left, like "Use the new Ruby 1.9 hash syntax", which I ignored because raven-ruby still supports Ruby 1.8.7, and "Cyclomatic complexity for to_hash is too high", which would require a bigger refactoring.

@@ -186,8 +189,8 @@
},
:server_name => 'foo.local',
:configuration => config
}).to_hash()
}
).to_hash
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe removing that explicit "{}" is causing a build failure on 1.8.7.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I'll check and fix that

@nateberkopec
Copy link
Contributor

I think most of these changes are OK - up to @dcramer if he thinks the obfuscation of git history due to this is worth it.

@ggarnier
Copy link
Contributor Author

ggarnier commented Dec 4, 2013

Done! I also added mime-types as development dependency, because coveralls gem require rest-client that requires mime-types, and the latest version of it (2.x) doesn't support Ruby 1.8.7. It would be a good idea to add Gemfile.lock to git to prevent this kind of problems.

@nateberkopec
Copy link
Contributor

+1 to merge now, again with the caveat of the effects on git history as mentioned above. This is a big change, so @dcramer ?

@dcramer
Copy link
Member

dcramer commented Dec 4, 2013

@nateberkopec no objections here

nateberkopec added a commit that referenced this pull request Dec 4, 2013
A lot of improvements on formatting and syntax best practices
@nateberkopec nateberkopec merged commit 3563a6b into getsentry:master Dec 4, 2013
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.

3 participants