Re: [PATCH 3/4] diff: change scope of the function count_lines()
- From
Shourya Shukla <shouryashukla.oo@gmail.com>
- Date
- Jul 4, 2020, 15:28 UTC
- Message-ID
- <20200704152802.GA46571@konoha>
- In-Reply-To
- <nycvar.QRO.7.76.6.2007031659580.50@tvgsbejvaqbjf.bet>
On 03/07 05:07, Johannes Schindelin wrote:
Show 19 quoted lines
> Hi Shourya, > > On Fri, 3 Jul 2020, Shourya Shukla wrote: > > > From: Prathamesh Chavan <pc44800@gmail.com> > > > > Change the scope of function count_lines for allowing the function > > to be reused in other parts of the code as well. > > It may be just me, but I'd rather see the word "visibility" instead of > "scope" here. I mistook the subject line to indicate that the function is > changed to serve an (at least slightly) different purpose than before, > which is not actually the case. > > Another alternative to "visibility" might be to imitate existing commit > messages, such as e4cb659ebdd (diff: export diffstat interface, > 2019-11-13), 22184497a36 (factor out refresh_and_write_cache function, > 2019-09-11) or ef283b3699f (apply: make parse_git_diff_header public, > 2019-07-11).
These are some excellent commit messages! I will take inspiration from these. Thanks Dscho! :)
Show 5 quoted lines
> In addition, as with all such changes, we need to be careful to consider > whether unrelated function names coming in from system headers might > clash. In this case, I think `count_lines()` is a bit too generic, but > will probably not clash. Personally, I would probably have opted for > `count_lines_in_string()`.
I do not see it clashing with any other functions. Do we need to change the name still (maybe to make the functions purpose even more clear)?