[Commented On] D9056: fix: include adjacent blank lines in ranges to be fixed

[hooper (Danny Hooper)]

hooper added a comment.


  The config changes are a big improvement, but I'm still not sure if it's worth supporting. One thing to note is that this feature doesn't help users who have multiple formatters behind a wrapper script configured as a single fixer tool. Configuring it as multiple fixer tools is possible, but not always practical, since it may duplicate some logic between the hg config and something else (sorry to be vague about that).

INLINE COMMENTS

> hooper wrote in fix.py:607
> We can additionally make the extra work conditional on the config value.

The O(lines) initialization of blanks2 is still happening whether or not we use it.

> fix.py:625
> +        # Produce a line range whenever the two sources differ and EITHER there
> +        # is an addition/modification OR :emitdeletedranges is not 'none'.
> +        if kind == b'!' and (firstline != lastline or all_adjacent or

"Produce a line range for every addition and modification. Conditionally produce ranges for deletions."

> fix.py:626
> +        # is an addition/modification OR :emitdeletedranges is not 'none'.
> +        if kind == b'!' and (firstline != lastline or all_adjacent or
> +                             (only_blanks and

Might be more readable to give this expression a name:

deletion = (firstline == lastline)

> fix.py:638
> +                    range_[0] - int(any(blanks2[firstline - 1: firstline])),
> +                    range_[1] + int(any(blanks2[lastline: lastline + 1])),
> +                )

I would prefer ":" instead of ": ", or at least " : ".

> fix.py:640
> +                )
> +            ranges.append(range_)
>      return ranges

I still suggest combining adjacent/overlapping ranges.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D9056/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D9056

To: msuozzo, hooper, #hg-reviewers
Cc: pulkit, mercurial-patches
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.mercurial-scm.org/pipermail/mercurial-patches/attachments/20201022/f9f71390/attachment-0002.html>