10000 Remove redundant safe navigation operator by tagliala · Pull Request #8528 · activeadmin/activeadmin · GitHub
[go: up one dir, main page]

Skip to content

Remove redundant safe navigation operator #8528

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

Merged
merged 1 commit into from
Nov 2, 2024

Conversation

tagliala
Copy link
Contributor
@tagliala tagliala commented Nov 2, 2024

Remove redundant safe navigation operator (&.) in result.to_s.strip.present? check, as to_s handles nil gracefully by converting it to an empty string.

This makes the code cleaner and more idiomatic without altering behavior.

Ref: https://ruby-doc.org/core-3.1.0/Object.html#method-i-to_s

@@ -24,7 +24,7 @@ def DISPLAY_NAME_FALLBACK.inspect
def display_name(resource)
unless resource.nil?
result = render_in_context(resource, display_name_method_for(resource))
if result.to_s&.strip&.present?
if result.to_s.strip.present?
Copy link
Contributor Author
@tagliala tagliala Nov 2, 2024

Choose a reason for hiding this comment

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

unless I'm missing something and in some circumstances to_s can be nil (maybe override to_s, but that is a user problem)

It may happen in AR when you do something like

class MyModel < ApplicationRecord
  def to_s
    name
  end
end

and name is nil.

But this should not be expected and is hiding a problem

This should be implemented with delegate :to_s, to: :name

Feel free to reject this change

Copy link
Member

Choose a reason for hiding this comment

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

I made this update in #6342 but I don't recall why I used the safe navigation operator. Likely just out of caution without really knowing that nil.to_s will return an empty string so it is redundant as you say. This looks good to me and makes sense to change. Thank you.

Copy link
codecov bot commented Nov 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.11%. Comparing base (b86bbbf) to head (e49f86e).
Report is 12 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #8528   +/-   ##
=======================================
  Coverage   99.11%   99.11%           
=======================================
  Files         141      141           
  Lines        4076     4076           
=======================================
  Hits         4040     4040           
  Misses         36       36           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member
@javierjulio javierjulio left a comment

Choose a reason for hiding this comment

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

Thanks!

@tagliala tagliala force-pushed the chore/remove-redundant-safe-navigation branch from cbd105e to 0594538 Compare November 2, 2024 17:41
Remove redundant safe navigation operator (`&.`) in
`result.to_s.strip.present?` check, as `to_s` handles `nil` gracefully
by converting it to an empty string.

This makes the code cleaner and more idiomatic without altering
behavior.

Ref: https://ruby-doc.org/core-3.1.0/Object.html#method-i-to_s
@tagliala tagliala force-pushed the chore/remove-redundant-safe-navigation branch from 0594538 to e49f86e Compare November 2, 2024 17:42
@tagliala tagliala merged commit a19acfb into master Nov 2, 2024
21 checks passed
@tagliala tagliala deleted the chore/remove-redundant-safe-navigation branch November 2, 2024 17:46
tagliala added a commit that referenced this pull request Nov 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3D28
Development

Successfully merging this pull request may close these issues.

2 participants
0