Re: [PATCH v2 6/6] version: introduce osversion.command config for os-version output
- From
Usman Akinyemi <usmanakinyemi202@gmail.com>
- Date
- Jan 20, 2025, 19:08 UTC
- Message-ID
- <CAPSxiM9QoCy7e+R9GAoELP1i+bssPoq2ogXPnbW59-ta6wjdoQ@mail.gmail.com>
- In-Reply-To
- <CAPig+cSE=2jZvTUHsBnRRMizC3pxRwyud+TGDV7iw=QNmFmycQ@mail.gmail.com>
On Tue, Jan 21, 2025 at 12:11 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 32 quoted lines
> > On Mon, Jan 20, 2025 at 1:17 PM Usman Akinyemi > <usmanakinyemi202@gmail.com> wrote: > > On Sat, Jan 18, 2025 at 3:14 AM Eric Sunshine <sunshine@sunshineco.com> wrote: > > > On Fri, Jan 17, 2025 at 5:47 AM Usman Akinyemi > > > <usmanakinyemi202@gmail.com> wrote: > > > > + if test_have_prereq !WINDOWS > > > > + then > > > > + printf "\nos-version=%s\n" $(uname -srvm | test_redact_non_printables) >>agent_and_long_osversion > > > > + fi && > > > > > > As an aid to future readers, please add an explanation either in the > > > commit message or as a comment here in the code explaining why Windows > > > is being singled out as special. > > > > The previous commit which introduced this has this information, > > can we do some form of referencing ? > > My main concern is that someone looking at this change in the future > -- who did not have the benefit of reading the cover letter or the > review discussion -- may have a hard time understanding why Windows is > singled out by this patch. As long as you give some sort of > explanation, whether in the code or in the commit message, then you > save that future user from having to figure it out on his or her own. > > So, your suggestion of referencing some other commit may work. > Augmenting the commit message of this patch with something along the > lines of: > > As with the previous commit, we skip the tests on Windows. > > may be enough to tell the reader where to look for the explanation.
Yeah, thanks, this looks better and clearer. I will add this in the next iteration if we agree to include the osversion.command config.
Thank you. Usman Akinyemi.