Skip to content

ref: Only obtain max retry count once #2521

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
May 27, 2025

Conversation

szokeasaurusrex
Copy link
Member

With the current implementation, we attempt to retrieve the max retry count from the environment or from the ini file on every access, which could lead to the user being warned multiple times if an invalid value is supplied. Instead, we should only obtain this count once and store it in the Config

Depends on:

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Introduce caching of the max retry count in Config to avoid repeated environment/INI lookups and warnings.

  • Add a max_retries field to Config and initialize it once.
  • Extract retry-count resolution into obtain_max_retry_count.
  • Simplify get_max_retry_count to return the stored value and update Clone and tests.
Comments suppressed due to low confidence (4)

src/config.rs:806

  • [nitpick] The tests only cover the default value path for max_retries. Add tests for environment variable override, INI file override, and invalid values to ensure all branches of obtain_max_retry_count are exercised.
max_retries: 0,

src/config.rs:527

  • [nitpick] Consider renaming obtain_max_retry_count to get_max_retry_count_from_config or load_max_retry_count to align with the get_ prefix convention used by other helper functions.
fn obtain_max_retry_count(ini: &Ini) -> u32 {

src/config.rs:533

  • The formatting string uses {MAX_RETRIES_ENV_VAR} as if it were a named placeholder but no named argument is provided. Change to a positional placeholder and pass the constant, for example: "Ignoring invalid {} environment variable: {}", MAX_RETRIES_ENV_VAR, e.
"Ignoring invalid {MAX_RETRIES_ENV_VAR} environment variable: {}",

src/config.rs:543

  • Similar formatting issue here: use a positional placeholder and pass MAX_RETRIES_INI_KEY, e.g.: "Ignoring invalid {} ini key: {}", MAX_RETRIES_INI_KEY, e.
warn!("Ignoring invalid {MAX_RETRIES_INI_KEY} ini key: {}", e);

@szokeasaurusrex szokeasaurusrex force-pushed the szokeasaurusrex/no-error-max-retries branch from b7a8af5 to 7ee7cad Compare May 27, 2025 10:31
@szokeasaurusrex szokeasaurusrex force-pushed the szokeasaurusrex/max-retries-only-once branch from 4284d77 to c9f2fee Compare May 27, 2025 10:32
Base automatically changed from szokeasaurusrex/no-error-max-retries to master May 27, 2025 11:12
With the current implementation, we attempt to retrieve the max retry count from the environment or from the ini file on every access, which could lead to the user being warned multiple times if an invalid value is supplied. Instead, we should only obtain this count once and store it in the `Config`
@szokeasaurusrex szokeasaurusrex force-pushed the szokeasaurusrex/max-retries-only-once branch from c9f2fee to 270b6ae Compare May 27, 2025 11:17
@szokeasaurusrex szokeasaurusrex enabled auto-merge (squash) May 27, 2025 11:17
@szokeasaurusrex szokeasaurusrex disabled auto-merge May 27, 2025 11:17
@szokeasaurusrex szokeasaurusrex merged commit dbdeba7 into master May 27, 2025
18 checks passed
@szokeasaurusrex szokeasaurusrex deleted the szokeasaurusrex/max-retries-only-once branch May 27, 2025 12:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants