Skip to content

refactor: remove uses of Option in new_with_config() #978

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

pickx
Copy link

@pickx pickx commented Jun 14, 2025

so this PR is rather large in terms of LoC, and I hope this is something you even want.
if not, that's totally fine and feel free to reject this.

while working on related features, I noticed the following:

  • several structs have a new_with_config() function which takes Option<&config::Config>. these functions then proceed to try and read the config in order to parse the values.
  • this is done via an assortment of get_*-or-use-default functions. these all need to handle the case where the config itself is None. what's more, if the config is indeed None, the default is chosen for all of them.
  • unit tests all go through this mechanism where the same code paths are used for both "config available" and "config unavailable". in particular, out of 272 tests that go through module_test(), 245 pass None as config which (imho) is just noise.

thus, this PR

  1. separates these constructors to either "config is available" new_with_config() which takes &Config instead of Option<&Config>, or "config is unavailable, so use defaults for everything"... in which case I just impl Default::default().
  2. changes the get_ functions to also take a &Config instead of Option<&Config>`, which simplifies the logic
  3. basic refactors of some of the logic in the areas I've touched.
  4. removes some now-unnecessary testing logic which creates dummy data
  5. separates module_test() to "with config" and "without config" versions, which removes the None parameter from a whole bunch of existing tests

in the "default" implementations, we can also do something that reuses the default values between the "load from config" and "load defaults" constructors, if you'd like.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 15656777377

Details

  • 148 of 152 (97.37%) changed or added relevant lines in 14 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage decreased (-0.02%) to 97.566%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/config.rs 9 10 90.0%
src/config/git_config.rs 23 26 88.46%
Files with Coverage Reduction New Missed Lines %
src/config.rs 1 87.1%
Totals Coverage Status
Change from base Build 15519682599: -0.02%
Covered Lines: 4930
Relevant Lines: 5053

💛 - Coveralls

@MitMaro
Copy link
Owner

MitMaro commented Jun 15, 2025

👋🏼 Thanks @pickx for the changes!

I've taken a brief look, I may request some of these be broken out into separate pull requests.

Some of the changes I am very much aligned on, while others less so, so I will try to add some comments around different things.

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.

3 participants