Re: [PATCH v2 0/2] [RFC][Outreachy] advice: refactor advise API
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Feb 17, 2020, 00:09 UTC
- Message-ID
- <xmqqftfam5ux.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <pull.548.v2.git.1581889150.gitgitgadget@gmail.com>
"Heba Waly via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 5 quoted lines
> Main changes in V2: 1- Rename advise_ng to advise_if_enabled. 2- Add a new > advise_enabled() helper. 3- Add a list of config variables names to replace > advice_config[] (used by list_config_advices()). 4- Send an enum parameter > to the new advise helpers instead of strings. 5- Extract vadvise() from > advise() and advise_if enabled().
Nice. There is not much point for me to correct mistakes in the title of the cover letter, but anyway... I think these changes are not "refactoring" the API, but these are enhancing, extending or even revamping the API.
Show 9 quoted lines
> To introduce a new advice message, the caller needs to:
>
> * Add a new_advice_type to 'enum advice_type'
> * Come up with a new config variable name and add this name to
> advice_config_keys[]
> * Call advise_if_enabled(new_advice_type, "advice message to be printed")
> * Or call advice_enabled(new_advice_type) first and then follow is by
> advice("advice message to be printed") as explained earlier.
> * Add the new config variable to Documentation/config/advice.txtAnd I see that now you are going all the way to discard the string-based keys to enumeration, I think this deserves to be called "revamp advise API".
> In the future, we can investigate generating the documentation from the list > of config variables or vice versa to make introducing a new advice much > easier, but this approach will do it for now.
Yup. One step at a time.