emacs-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [SUSPECTED SPAM] Re: [Emacs-diffs] scratch/widen-less a4ba846: Repla


From: Eli Zaretskii
Subject: Re: [SUSPECTED SPAM] Re: [Emacs-diffs] scratch/widen-less a4ba846: Replace prog-widen with consolidating widen calls
Date: Sat, 09 Dec 2017 12:47:04 +0200

Sorry for the delay in responding: 3 days of absence took their toll.

> Cc: address@hidden
> From: Dmitry Gutov <address@hidden>
> Date: Wed, 6 Dec 2017 20:41:21 +0200
> 
> Yes, this code has been in Emacs source tree for two years. But:
> 
> It's only used is one line in antlr-mode.el, which wasn't supposed to 
> start using it before Emacs 26 was released anyway. But it's easy to change.
> 
> On the flip side, prog-indentation-context is *only* well-supported in 
> python-mode. Not in css-mode, js-mode, ruby-mode or others. In those, 
> you still have to combine its binding with narrow-to-region, essentially 
> obviating the need for the second element in the list. In short: we have 
> the code, and even Emacs, after the said 2 years, isn't supporting it 
> properly.
> 
> PREVIOUS-CHUNKS isn't supported anywhere either. And, in that case, 
> there's no code corresponding to it. Just a few pieces of documentation.

I think we are failing to communicate.  "Not supported anywhere" is
not a reason good enough in my book to remove something.  We have
there a feature that is a super-set of what you need for the MMM
related support, so a much better way forward is to leave that stuff,
since it should transparently support your use case, or almost so.
Arguing that there's a good reason for removing
prog-indentation-context would need to show how keeping it _precludes_
some of what you are trying to do, or at least makes it unreasonably
hard.

And please keep in mind that 2 years without any additional users is
not long enough to prove that this feature is useless.  Let's also
keep in mind that this feature was reviewed at the time and admitted
into Emacs, which means Stefan did think at least back then that it
could be useful in practice.  We should give people a chance before
deciding it is completely useless and worthy of removal.

> No, the widen calls are conditioned on whether we want all 
> indent-line-function calls to start in fully widened state. Which seems 
> like an obvious improvement. Point is, they won't be allowed to call 
> 'widen' themselves, so the addition of 'widen' to indent-for-tab-command 
> and indent-according-to-mode makes them act on the whole buffer in the 
> simple case.

Whether we should unconditionally call 'widen' there is now a subject
of a separate discussion.  For the purposes of this discussion, the
only issue that should matter is whether using 'prog-widen' instead of
'widen' in those places will get in the way of the MMM support.  I
think it doesn't.

> > What I see in the branch is this:
> > 
> >    1) the calls to prog-widen are replaced with calls to widen.
> 
> No, they are not. The calls to prog-widen (or widen) made in indentation 
> related code are removed.

I don't think this distinction is important.  I presumed that you are
removing those widen calls because you added similar calls on higher
levels, and didn't mention those removals.  If that's not so, please
point to specific parts of the patch where the widen calls are removed
without that assumption.

> The only place in the diff where prog-widen was replaced with widen, is 
> where it probably shouldn't have been in the first place (that code is 
> not inside indent-line-function; only tangentially related).

And I'm proposing to leave that prog-widen call intact, because by
default it's the same as calling 'widen' in the first place.  By
keeping the prog-widen call there, we make the change run lower risk
of being backward incompatible.

> >    2) some calls to widen are added
> 
> In code that later either calls indent-line-function or 
> beginning-of-defun-function.

This is about widening unconditionally, so it's now unrelated to the
current discussion.

> >    3) prog-indentation-context is removed
> 
> Yup.

And I see no reason for removing it, because if not set to any non-nil
value, it is harmless.  If you can explain why leaving it contradicts
support for MMM, please do.

> >    4) prog-first-column the function is removed, and its calls replaced
> >       with accessing the (existing) name-sake variable
> 
> Yes. It's not a hard requirement, but there doesn't seem much benefit in 
> keeping it a function. And if it's a function, what will it return?

I don't think it matters what it will return.  What matters is that
(a) keeping a function doesn't interfere with the MMM support in any
way I could see; (b) keeping a function makes the changes fully
backward-compatible; and (c) keeping a function will allow future
extensions where the value returned is not trivially taken from a
variable.

> > For 4), we already agreed that keeping a function is better.
> 
> I did not. Allow me to post a quote from my other message:
> 
> If we keep it a function, do we also have a variable prog-first-column?

We need to store the value somewhere, or have a way of recomputing it.
Whether that is done by keeping a variable, and whether that variable
is called the same as the function, are secondary issues; the primary
issue IMO is why remove a function that is already there, and is the
documented interface for accessing the value.  I'd again prefer
keeping the stuff unchanged, if just for the reason that this is how
the code was submitted 2 years ago, and this is how it was approved.
It's not the first time that we have a variable named the same as a
function.  But if there are good reason to keep the function, while
renaming the variable, I will probably agree to renaming the variable
much easier than I'd agree to removing the function.

> Then, some consumers might have doubt which ones to use, and might opt 
> to reference the variable. In that case, we lose all benefits of having 
> it a function (like making its implementation somehow smarter later), 
> and the only benefit remaining is backward compatibility of having a 
> function with that name.
> 
> But! Like with PREV-CHUNKS, prog-first-column (and the later prog-widen) 
> are supposed to be used by the major modes only. There are no references 
> to either of these functions in the latest antlr-mode.el, and there 
> shouldn't be.
> 
> And as long as all the calls to that function are inside Emacs, we're 
> free to change it however, including turning it into a variable.

These all are very weak reasons in my eyes.  Keeping the documented
interfaces stable and backward-compatible is a much stronger argument,
and IMO in this case it easily overcomes the above considerations.

> > Since prog-widen already calls widen if prog-indentation-context is
> > nil (which it is by default), calling prog-widen without setting up
> > prog-indentation-context will just call widen; this magically takes
> > care of 1).
> > 
> > For 3), if we leave prog-indentation-context in the code, and also
> > allow direct calls to widen in modes which don't want to use the
> > context, we are not losing anything, while leaving the option of using
> > the context to those modes which will want that.  This currently
> > cannot be used by MMM (AFAIU), but other features which need embedded
> > code, such as ANTLR, could still use it, and even MMM will be able to
> > do that if it is extended.
> 
> As I hopefully described, the above does not make sense, unfortunately.

You explained that you consider the design and implementation of
prog-indentation-context unnecessarily complex, and that a simpler
design and implementation would be more elegant, given the current
practices in related modes.  You have _not_ explained why keeping the
prog-indentation-context stuff would prevent us from supporting MMM
and similar features as easily as under your proposed changes.  If
there are such aspects of prog-indentation-context, please point them
out, because those are _the_only_ ones which would convince me to
remove prog-indentation-context.

> > 2) can be taken care of as indicated above, thus avoiding any possible
> > effects on existing behaviors when MMM is not active.
> 
> Not sure I understand you here.

That's the "should we unconditionally widen" issue, which I now took
to a separate thread.  It was also my attempt to keep the changes safe
enough to land them on the release branch.  So we can forget about
this part for the purposes of this discussion.

Thanks.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]