Lint/SafeNavigationChain and try

[MOZGIII]

Cop Lint/SafeNavigationChain does not allow try and try!.

---

Expected behavior

try and try! should be allowed after &. in Rails.

Actual behavior

Cop does not allow try and try! after &. in Rails.

Steps to reproduce the problem

$ echo 'a&.b.try(:c)' | rubocop -s demo
Inspecting 1 file
W

Offenses:

demo:1:5: W: Do not chain ordinary method call after safe navigation operator.
a&.b.try(:c)
    ^

1 file inspected, 1 offense detected

RuboCop version

$ rubocop -V
0.47.0 (using Parser 2.3.3.1, running on ruby 2.3.1 x86_64-linux)

[bbatsov]

I see no reason why someone should write code like this. Some rationale would be useful...

[MOZGIII]

Here: property&.params.try(:purpose)&.code

params is a polymorphic model here, thus can either have or not have purpose method.
This is the most compact way to write the code for a particular situation.

[MOZGIII]

This cop detected a lot of problems in our code, however some of them are false-positives due to this issue.
It's very useful, but would be much better with the fix.

[MOZGIII]

Also, it's just perfectly fine to write something like nil.try(...) or nil.try!(...) in Rails, so if it needs to be checked it should probably be done in a separate cop.
Idea for a cop btw: use &. instead of .try!(...) where possible.

[mikegee]

It would be nice if more folks contributed their thoughts on this. I'm not convinced the change requested improves the code.

In the particular case presented: property&.params.try(:purpose)&.code. I think defining the purpose method on all the objects params might return is a better approach.

[pocke]

it is reasonable.

Idea for a cop btw: use &. instead of .try!(...) where possible.

We can use Rails/SafeNavigation cop for the purpose.
https://github.com/bbatsov/rubocop/blob/master/lib/rubocop/cop/rails/safe_navigation.rb

In the particular case presented: property&.params.try(:purpose)&.code. I think defining the purpose method on all the objects params might return is a better approach.

Yeah, however, I think it is out of scope for this cop.
The refactoring should be suggested by other cop or other analyser.

[bbatsov]

I agree.

…

On Tue, Jan 17, 2017 at 11:21 Masataka Kuwabara ***@***.***> wrote: it is reasonable. Idea for a cop btw: use &. instead of .try!(...) where possible. We can use Rails/SafeNavigation cop for the purpose. https://github.com/bbatsov/rubocop/blob/master/lib/rubocop/cop/rails/safe_navigation.rb In the particular case presented: property&.params.try(:purpose)&.code. I think defining the purpose method on all the objects params might return is a better approach. Yeah, however, I think it is out of scope for this cop. The refactoring should be suggested by other cop or other analyser. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#3915 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAGVyl3tFP3pNc5Sc1y9PG9bvpnUMt7Rks5rTEG4gaJpZM4LkhbT> .

[igas]

Does it make sense to add more exceptions? for example:

foo&.bar.presence
foo&.bar.present?
foo&.bar.nil?

[MOZGIII]

I guess so. Any method that can be used in nil? is technically legal, so all of them should be allowed.

[pocke]

Does it make sense to add more exceptions? for example:

.present? and .nil? have been added to exceptions already. However, .presence hasn't been added yet.

The following methods are added by RoR into NilClass.

acts_like?
as_json
__binding__
blank?
bullet_key
byebug
class_eval
debugger
deep_dup
duplicable?
gem
html_safe?
in?
instance_values
instance_variable_names
load_dependency
presence
presence_in
present?
pretty_inspect
pretty_print
pretty_print_cycle
pretty_print_inspect
pretty_print_instance_variables
primary_key_value
pry
psych_to_yaml
require_dependency
require_or_load
suppress_warnings
to_json
to_param
to_query
to_yaml
to_yaml_properties
try
try!
unloadable
with_options

However, the cop supports only blank?, present? and try.

Should the cop support the above all methods?

[bbatsov]

Maybe we should just make this configurable and let people add there whatever they want. I myself hate a style of programming that assumes nils can pop up everywhere...

[bbatsov]

At any rate - I wouldn't go overboard with this...

[MOZGIII]

I agree. It's currently almost impossible to deduce a set of methods that are allowed to be called after &. or .try(...).
It will also vary from project to project.
But sane defaults are great anyway.

[MOZGIII]

Could you please push release to rubygems?

[bbatsov]

I don't plan to issue a new release soon. This is a tiny issue that doesn't warrant a special release. Just build the gem from master in the mean time.

…

On Fri, Jan 20, 2017 at 18:59 MOZGIII ***@***.***> wrote: Could you please push release to rubygems? — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#3915 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAGVyu5oPcePwmnm45QtmG1fckgnYck8ks5rUKGWgaJpZM4LkhbT> .

[bbatsov]

P.S. My decision might change if more bugs get fixed in the next couple of days.