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

Configuration methods do not have consistent naming #764

Closed
suaviloquence opened this issue Apr 19, 2024 · 3 comments · Fixed by #765
Closed

Configuration methods do not have consistent naming #764

suaviloquence opened this issue Apr 19, 2024 · 3 comments · Fixed by #765
Labels
C-enhancement Category: raise the bar on expectations E-help-wanted Call for participation: Help is requested to fix this issue. E-mentor Call for participation: Mentorship is available for this issue.

Comments

@suaviloquence
Copy link
Contributor

suaviloquence commented Apr 19, 2024

Question on method naming: it seems like we now have a set_level() method where before we had a with_log_level() method. Is this divergence intentional, or should we instead have either set_log_level() or with_log_level() as before? Similar question about with the other set_...() methods, as opposed to the existing with_...() methods' naming convention.

The GlobalConfig::set_level method already existed before this PR, so I left it unchanged. Additionally, since we used to have that and the set_color_choice method, I used the set_ prefix for GlobalConfig setters. I agree that it's not consistent between GlobalConfig and Check, but they are both internally consistent. Should we use the same prefix for everything? If so, in here, or in a different PR?

Originally posted by @suaviloquence in #737 (comment)

If we want all the methods to be consistent across Check and GlobalConfig, we need to choose:

  • do we want the with_ (found in Check) prefix or the set_ (from GlobalConfig) prefix?
  • do we want set_log_level or set_level?

I definitely prefer log_level over level, and I have a slight preference for set over with. What do others think?

@obi1kenobi obi1kenobi added C-enhancement Category: raise the bar on expectations E-help-wanted Call for participation: Help is requested to fix this issue. E-mentor Call for participation: Mentorship is available for this issue. labels Apr 19, 2024
@obi1kenobi
Copy link
Owner

Sounds good, let's go with that!

Probably best to make this change now, before the next release, so that we only break the API once. Would you mind posting the updated migration guide together with the PR?

@suaviloquence
Copy link
Contributor Author

I can do that. One other thing: do we want to standardize the calling convention? Currently only set_level takes an owned Self (and it doesn't use it, we could use a &mut self). If we copy the Check convention, setter methods go fn(&mut self, ...) -> &mut self which I think makes a lot of sense.

@obi1kenobi
Copy link
Owner

Sounds good to me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: raise the bar on expectations E-help-wanted Call for participation: Help is requested to fix this issue. E-mentor Call for participation: Mentorship is available for this issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants