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

Upgrading some gems. #342

Closed
wants to merge 5 commits into from
Closed

Upgrading some gems. #342

wants to merge 5 commits into from

Conversation

vraravam
Copy link

@vraravam vraravam commented Dec 7, 2014

No description provided.

@coveralls
Copy link

Coverage Status

Coverage decreased (-31.48%) when pulling 94aca8d on vraravam:upgrading-gems into cca6eeb on FarmBot:master.

@simonv3
Copy link
Member

simonv3 commented Dec 7, 2014

Hey @vraravam, thanks so much for the PR!

It looks like bumping the version for active_models_serializers breaks the API. Any thoughts what the difference would be?

…s the app - will investigate later tomorrow.
@vraravam
Copy link
Author

vraravam commented Dec 7, 2014

I have reverted that gem alone to the previous version and locked it in the Gemfile. Please verify/consider for merge.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling d121bc7 on vraravam:upgrading-gems into cca6eeb on FarmBot:master.

… can work out of the box (also changed hound config to point to this file). Fixed lots of rubocop warnings.
garden_crop = Garden.find(params[:garden_id]).
garden_crops.find(params[:id])
garden_crop = Garden.find(params[:garden_id])
.garden_crops.find(params[:id])

Choose a reason for hiding this comment

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

Place the . on the previous line, together with the method call receiver.

@coveralls
Copy link

Coverage Status

Coverage decreased (-25.84%) when pulling d602555 on vraravam:upgrading-gems into cca6eeb on FarmBot:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-25.84%) when pulling d602555 on vraravam:upgrading-gems into cca6eeb on FarmBot:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-31.78%) when pulling 71d2c33 on vraravam:upgrading-gems into cca6eeb on FarmBot:master.

@simonv3
Copy link
Member

simonv3 commented Dec 8, 2014

Hey @vraravam,

Thanks again! A good idea is to run the tests (rspec spec) locally before pushing to the branch. That way you can catch any of these errors yourself.

It's also better if you don't conform files that you haven't worked on for the PR to a style guide (like for Hound), usually it ends up creating problems (like in this case). The idea is that slowly those will conform to the style guide as code gets refactored, it's usually less messy to do it that way.

@RickCarlino
Copy link
Contributor

Hi @vraravam, thanks for the PR, we always appreciate the help.

Unfortunately, I don't feel comfortable merging this PR and am going to close it. Many of your changes help us conform to a style guide, which is definitely a good thing, but I do not think it is appropriate for us to change working production code for the sake of complying to a style guide. This is especially true if the changes are being made by someone who did not write the original code and may not have a complete understanding of any underlying complexity.

In the past this has always (not sometimes) resulted in higher occurrences of errors on the production site.

In the future, please update the style of code only when implementing new features or bug fixes. Sorry for any confusion.

@RickCarlino RickCarlino closed this Dec 8, 2014
@vraravam
Copy link
Author

vraravam commented Dec 8, 2014

hi Rick,
I understand your concerns. I have gone ahead and deleted my repo as well.
In case I decide to contribute to this project, I will take a look again at
how to do so.

On Mon, Dec 8, 2014 at 8:58 AM, Rick Carlino notifications@github.com
wrote:

Closed #342 #342.


Reply to this email directly or view it on GitHub
#342 (comment).

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

5 participants