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

Standardize use of ENV.fetch for strings / numbers #49244

Conversation

ksylvest
Copy link
Contributor

@ksylvest ksylvest commented Sep 12, 2023

Motivation / Background

Usage of ENV.fetch is inconsistent throughout the repo. Given a slight mem / perf optimization this standardizes to use fetch w/ the fetch(key, default) syntax everywhere the default is an integer / string. This also matches the default preferred format in RuboCop per:

https://www.rubydoc.info/gems/rubocop/RuboCop/Cop/Style/RedundantFetchBlock

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug or add a feature.
  • CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

Simplifies the legibility of the fallback for non-env. It also avoids
needing to allocate a block.
@rafaelfranca
Copy link
Member

Thank you for the pull request, but we don't accept cosmetic changes.

@ksylvest ksylvest deleted the ksylvest/standardize-env-fetch-usage-for-strings-and-integers branch September 12, 2023 20:38
@zzak
Copy link
Member

zzak commented Sep 13, 2023

Given a slight mem / perf optimization

I can see an argument to change this if there is an actual impact on performance, but I think most of the changes here might only affect startup?

Do you have a benchmark for the before and after?

As we disable all of the rubocop defaults here:

rails/.rubocop.yml

Lines 10 to 13 in 699dfdb

# RuboCop has a bunch of cops enabled by default. This setting tells RuboCop
# to ignore them, so only the ones explicitly set in this file are enabled.
DisabledByDefault: true
SuggestExtensions: false

I wondered if there would be value in explicitly disabling rules, like this, to avoid similar PRs:

@ksylvest
Copy link
Contributor Author

ksylvest commented Sep 13, 2023

@zzak the delta in performance is likely fairly tiny given the non-repeated execution and my motivation is/was to avoid needing to disable / fix when integrating rubocop on freshly generated apps.

For reference, here's a quick benchmark (required fairly huge sample to see a difference):

require 'benchmark'
N = 5_000_000
Benchmark. bmbm do |benchmark|
  benchmark.report("w/ fallback") { N.times { ENV.fetch('KEY', 'fallback') } }
  benchmark.report("w/ block") { N.times { ENV.fetch('KEY') { 'fallback' } } }
end
                  user     system      total        real
w/ fallback   1.721472   0.018988   1.740460 (  1.800765)
w/ block      2.120957   0.019862   2.140819 (  2.248313)

@zzak
Copy link
Member

zzak commented Sep 13, 2023

FWIW, I liked Matthew's idea of adding a linter option to the generator which autofixes any violations when running "rails new" but this is probably not a tiny patch

@zzak zzak mentioned this pull request Feb 15, 2024
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants