Frozen string literal #1790
Frozen string literal #1790
Conversation
…resent Added specs
Added rubocop_todo
Refactor specs
We do not validate if all empty We skip validation if parents is optional
# Conflicts: # CHANGELOG.md # lib/grape/validations/params_scope.rb
|
We use RuboCop that adds I could misunderstand what this does, but otherwise I propose we don't merge this. If we were to merge this, I'd want to see rubocop rules changed to enforce this header everywhere and remove the freeze calls. |
|
The I'm totally fine with removing all |
|
I think if we move forward with 2.3 minimum then yes, but even though support has ended in June I think we're ok being a bit behind. In my own org we're running a ton of Ruby 2.2 right now. @ericproulx I think you should turn this PR into exactly what you propose and lets see what other maintainers say? @namusyaka for example? |
|
Thanks for your contribution. But I don't think this changes improve performance largely, and not only that, this has possibility of leading new painful for us. I don't say that your change isn't reasonable however we should consider the switching timing and the sustainability of the grape for users. |
There was a problem hiding this comment.
If you would like to go on the issue, you should take a look at Rubocop configuration.
https://github.com/rubocop-hq/rubocop/blob/master/manual/cops_style.md#stylefrozenstringliteralcomment
| @@ -35,16 +35,17 @@ def pattern_options | |||
| end | |||
There was a problem hiding this comment.
Why don't you add the comment in this file?
There was a problem hiding this comment.
I guess I forgot :(
# Conflicts: # CHANGELOG.md # lib/grape/util/inheritable_values.rb # lib/grape/util/reverse_stackable_values.rb # lib/grape/util/stackable_values.rb # lib/grape/validations/validators/all_or_none.rb # lib/grape/validations/validators/at_least_one_of.rb # lib/grape/validations/validators/exactly_one_of.rb # lib/grape/validations/validators/mutual_exclusion.rb
|
Is running |
|
|
||
| pattern + suffix.to_s | ||
| new_pattern + suffix.to_s |
There was a problem hiding this comment.
Why don't we do patter = pattern + or something like that within the condition, instead of duplicating it every time?
|
Let us know when this is ready to merge. Don't forget CHANGELOG. Maybe squash? |
|
@namusyaka It does improve performance https://www.mikeperham.com/2018/02/28/ruby-optimization-with-one-magic-comment/ 🙂 |
|
I'm closing this PR, I opened another one #1941 |
Hi,
Since ruby 2.3, we can add the comment
# frozen_string_literal: trueat the start of the files to make all strings literals frozen by default. Since I'm using your gem on ruby 2.3, I thought it would be a great idea.