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.

Sign up for GitHub

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

Jump to bottom

Add frozen_string_literal pragma to ruby files #305

Merged
merged 2 commits into from
Mar 19, 2018
Merged

Add frozen_string_literal pragma to ruby files #305

merged 2 commits into from
Mar 19, 2018

Conversation

krzysiek1507
Copy link
Contributor

brendon
brendon requested changes Mar 15, 2018
View reviewed changes
Copy link
Owner

@brendon brendon left a comment

Choose a reason for hiding this comment

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

@swanandp, are you happy with this change? Is there a way to define this globally within our gem?

lib/acts_as_list/active_record/acts/position_column_method_definer.rb Outdated
@@ -21,11 +23,11 @@ def self.define_class_methods(caller_class, position_column)
end

define_singleton_method :decrement_all do
update_all_with_touch "#{quoted_position_column} = (#{quoted_position_column_with_table_name} - 1)"
update_all_with_touch "#{quoted_position_column} = (#{quoted_position_column_with_table_name} - 1)".dup
Copy link
Owner

Choose a reason for hiding this comment

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

Sorry, I'm feeling dense today :) Can you explain why we need to dup the string? :)

Copy link
Contributor Author

@krzysiek1507 krzysiek1507 Mar 15, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Here this string is concatenating with touch_record_sql but one cannot change a frozen string. :)

Copy link
Owner

Choose a reason for hiding this comment

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

Could we instead unfreeze (.dup?) updates in:

update_all(updates.dup << touch_record_sql)

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case, we won't mutate updates so maybe we can change it to concatenation?

update_all(updates + touch_record_sql)

Copy link
Owner

Choose a reason for hiding this comment

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

Yep, that't cleaner :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@brendon changed! :)

Copy link
Owner

Choose a reason for hiding this comment

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

Thanks @krzysiek1507, I'll just wait on @swanandp's check and then we'll be GTG.

@brendon
Copy link
Owner

brendon commented Mar 15, 2018

Thanks @krzysiek1507 :D

@krzysiek1507 krzysiek1507 force-pushed the feature/add-frozen-string-literal branch from 49fb4f6 to b4c478d Compare March 16, 2018 08:37
@brendon brendon requested a review from swanandp March 16, 2018 11:40
swanandp
swanandp approved these changes Mar 16, 2018
View reviewed changes
Copy link
Contributor

@swanandp swanandp left a comment

Choose a reason for hiding this comment

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

👍, when TravisCI confirms this is backward compatible

@krzysiek1507
Copy link
Contributor Author

TravisCI fails on Rails 5.2 and MySQL but it also fails on master.

@swanandp
Copy link
Contributor

I just checked, Rails 5.2 is a different issue not related to this one.

@brendon
Copy link
Owner

brendon commented Mar 17, 2018

We seem to be having suite failures for Rails 5.2 and certain rubies. I think it might be to do with rake versions? or something like that. I don't have time to look into it this weekend, but if either of you two can that would be cool :)

@krzysiek1507
Copy link
Contributor Author

The problem is mysql2. It is old and doesn't work with Rails 5.2 but 0.4.10 doesn't work with Rails < 4.2.

@brendon
Copy link
Owner

brendon commented Mar 18, 2018

Ah yes, sorry I already had this fixed in a branch. I'm just running it through the suite with the latest from master then I'll merge it in. Once that's done, can you update from master and we'll see if the suite passes still :)

@krzysiek1507
Copy link
Contributor Author

Hi @brendon! I'll do it! :)

@brendon
Copy link
Owner

brendon commented Mar 18, 2018

@krzysiek1507 Can you update from master, then it should be all green :)

@krzysiek1507 krzysiek1507 force-pushed the feature/add-frozen-string-literal branch from b4c478d to 7e02b81 Compare March 18, 2018 21:46
@krzysiek1507
Copy link
Contributor Author

@brendon all green!

@brendon brendon merged commit 26ef8c2 into brendon:master Mar 19, 2018
@brendon
Copy link
Owner

brendon commented Mar 19, 2018

Thanks @krzysiek1507 :) Great job!

@krzysiek1507 krzysiek1507 deleted the feature/add-frozen-string-literal branch March 19, 2018 06:50
@krzysiek1507
Copy link
Contributor Author

Thank you!

This was referenced Mar 19, 2018
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brendon brendon brendon requested changes

@swanandp swanandp swanandp approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@krzysiek1507 @brendon @swanandp