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

Simple refactoring on core #155

Closed
wants to merge 4 commits into from
Closed

Simple refactoring on core #155

wants to merge 4 commits into from

Conversation

pwener
Copy link
Contributor

@pwener pwener commented Feb 13, 2016

Why initializate_settings can't receive params to set @configurationInfo?

@rodrigosiqueira
Copy link
Member

First of all, thank you so much for your contribution. It is really important for us :)
Secondly, sorry for my delay, I'm moving from Brasilia to Sao Paulo since de last weekend.
...

@@ -20,10 +20,9 @@ def Setting.create
@@settings = new unless @@settings
return @@settings
end

Copy link
Member

Choose a reason for hiding this comment

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

Take care with white space.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry about that, I deletes the comment bellow and I put back again thinking it would be useful

Show error exception cause

Refactoring methods set_configuration

Breaking line with the number of characters exceeded

Remove white spaces
@rodrigosiqueira
Copy link
Member

@pwener it is ok your PR, I just have two observations:

  1. Remove white spaces (as I commented).
  2. Squash all your commits to one, because it is related to the same thing.
    Thanks :)

@pwener
Copy link
Contributor Author

pwener commented Feb 15, 2016

I'm failing to use squash, I will try to redo the branch.

@pwener pwener closed this Feb 15, 2016
@pwener
Copy link
Contributor Author

pwener commented Feb 15, 2016

#156

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

2 participants