Any exceptions for the 15-line rule?

Any exceptions for the 15-line rule?

[Dmitry Gutov]

I've received a pull request [0] today for company-mode's Clang backend
that's essentially a request to borrow some (fairly simple) code from
the auto-complete-clang project [1].

The latter is licensed under GPLv3+, but I'm pretty sure the author
(brianjcj AT gmail, not sure what's his full name) has not signed the
CA. And I'm not wild about the idea of waiting several months to add the
feature (that is, if the author even agrees to sign the CA).

The change itself is almost entirely dictated by the interface provided
by the clang program, so while I can tweak a thing here and there to try
to create an independent implementation, it's not like solving a complex
problem, which you can approach from different directions.

For example, the definition of `company-clang--lang-option' the patch
[2] adds is more or less what takes it over the 15 lines limit. I said
I'll try to rewrite it, but should I bother?

It's basically a set of information about what values the "-x" option
takes, and while I can replace the main `cond' with the less efficient
`replace-string', the result will still resemble the original very
strongly.

Speaking of one of the stated reasons behind the CA, this is not the
kind of code I'd ever expect FSF to go to court over.

--Dmitry

[0] https://github.com/company-mode/company-mode/pull/17
[1] https://github.com/brianjcj/auto-complete-clang
[2] https://github.com/company-mode/company-mode/pull/17.diff

Re: Any exceptions for the 15-line rule?

[Stefan Monnier]

> The latter is licensed under GPLv3+, but I'm pretty sure the author
> (brianjcj AT gmail, not sure what's his full name) has not signed the
> CA. And I'm not wild about the idea of waiting several months to add the
> feature (that is, if the author even agrees to sign the CA).

The change includes

   -    (with-temp-buffer
   +        (buf (get-buffer-create "*clang-output*"))
   +    (with-current-buffer buf (erase-buffer))
   +    (with-current-buffer buf

Which seems like it's making the code worse rather than better.
If you undo this undesirable part of the patch, it'll be closer to
the acceptable limit.

For company-clang--lang-option, I'd be tempted to use

   (defun company-clang--lang-option ()
     (if (eq major-mode 'objc-mode)
         (if (string= "m" (file-name-extension buffer-file-name))
             "objective-c" "objective-c++")
       (substring (symbol-name major-mode) 0 -5)))

With such cleanups, the patch seems acceptable as a "tiny change".
But please do ask for the CA as well (so the use of "tiny change" is
mostly a way to avoid having to wait for the CA to go through).


        Stefan

Re: Any exceptions for the 15-line rule?

[Dmitry Gutov]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> The latter is licensed under GPLv3+, but I'm pretty sure the author
>> (brianjcj AT gmail, not sure what's his full name) has not signed the
>> CA. And I'm not wild about the idea of waiting several months to add the
>> feature (that is, if the author even agrees to sign the CA).
>
> The change includes
>
>    -    (with-temp-buffer
>    +        (buf (get-buffer-create "*clang-output*"))
>    +    (with-current-buffer buf (erase-buffer))
>    +    (with-current-buffer buf
>
> Which seems like it's making the code worse rather than better.
> If you undo this undesirable part of the patch, it'll be closer to
> the acceptable limit.

Yes, I more or less reverted this piece in my mind already. :)

> For company-clang--lang-option, I'd be tempted to use
>
>    (defun company-clang--lang-option ()
>      (if (eq major-mode 'objc-mode)
>          (if (string= "m" (file-name-extension buffer-file-name))
>              "objective-c" "objective-c++")
>        (substring (symbol-name major-mode) 0 -5)))

Thanks, `substring' is better than `replace-match' I mentioned. But
still, should this be considered a full new implementation? Does
replacing `cond' with `if' in the inner condition make it a new piece of
code, as opposed to derivative one?

> With such cleanups, the patch seems acceptable as a "tiny change".
> But please do ask for the CA as well (so the use of "tiny change" is
> mostly a way to avoid having to wait for the CA to go through).

To be clear, who do I ask to sign the CA over the modified patch? The
auto-complete-clang author, or the person who looked at a few pieces
from that package and adapted them to (admittedly, fairly similar)
company-clang code?

Re: Any exceptions for the 15-line rule?

[Stefan Monnier]

> Thanks, `substring' is better than `replace-match' I mentioned. But
> still, should this be considered a full new implementation? Does
> replacing `cond' with `if' in the inner condition make it a new piece of
> code, as opposed to derivative one?

The purpose is just to "simplify" the code, rather than to obscure
the copyright.  In terms of copyright, it does reduce the amount of code
taken, indeed, but it's not a very significant difference.

>> With such cleanups, the patch seems acceptable as a "tiny change".
>> But please do ask for the CA as well (so the use of "tiny change" is
>> mostly a way to avoid having to wait for the CA to go through).
> To be clear, who do I ask to sign the CA over the modified patch? The
> auto-complete-clang author, or the person who looked at a few pieces
> from that package and adapted them to (admittedly, fairly similar)
> company-clang code?

In terms of who owns the copyright, the answer is probably "both", but
to the extent that it fits the "tiny change" criteria we don't need to
care too much (unless one or both of the authors already have
contributed code as a "tiny change" since those things are cumulative).

Assuming that we want company-mode and auto-complete to share more code
in the future, having the assignment of AC's author is a good idea.
As for the person who sent you the patch, it would also make sense to
get his/her assignment if there's a chance he'll contribute more in
the future.


        Stefan

PS: By the way, I think company-backends should be merged with (and/or
moved over to) completion-at-point-functions, and some of those backends
should be moved to their respective major modes.

Re: Any exceptions for the 15-line rule?

[Dmitry Gutov]

On 27.04.2013 17:28, Stefan Monnier wrote:
>> Thanks, `substring' is better than `replace-match' I mentioned. But
>> still, should this be considered a full new implementation? Does
>> replacing `cond' with `if' in the inner condition make it a new piece of
>> code, as opposed to derivative one?
>
> The purpose is just to "simplify" the code, rather than to obscure
> the copyright.  In terms of copyright, it does reduce the amount of code
> taken, indeed, but it's not a very significant difference.

I see.

>>> With such cleanups, the patch seems acceptable as a "tiny change".
>>> But please do ask for the CA as well (so the use of "tiny change" is
>>> mostly a way to avoid having to wait for the CA to go through).
>> To be clear, who do I ask to sign the CA over the modified patch? The
>> auto-complete-clang author, or the person who looked at a few pieces
>> from that package and adapted them to (admittedly, fairly similar)
>> company-clang code?
>
> In terms of who owns the copyright, the answer is probably "both", but
> to the extent that it fits the "tiny change" criteria we don't need to
> care too much (unless one or both of the authors already have
> contributed code as a "tiny change" since those things are cumulative).

> Assuming that we want company-mode and auto-complete to share more code
> in the future, having the assignment of AC's author is a good idea.

auto-complete-clang is a separate package from auto-complete (which has 
multiple authors, but the principal one already has CA on file: Tomohiro 
Matsuyama). I'll ask, although I think the only useful piece left we 
could borrow is converting the lightweight markup in completion 
candidates into syntax highlighting, and that I should be able to write 
in some different way without much difficulty.

> As for the person who sent you the patch, it would also make sense to
> get his/her assignment if there's a chance he'll contribute more in
> the future.

Ok, I'll ask.

> PS: By the way, I think company-backends should be merged with (and/or
> moved over to) completion-at-point-functions, and some of those backends
> should be moved to their respective major modes.

Yes, I remember the email that accompanied your company-capf patch, but 
so far I don't see a good way to go about it.

The interface provided by company-backends is considerably richer. How 
do we fit that inside completion-at-point-functions?

There's also the problem of keeping compatibility with already released 
Emacs versions. If we move the code, they won't be able to use it; if we 
keep it, it would create duplication.

Re: Any exceptions for the 15-line rule?

[Dmitry Gutov]

Here's another case, with even more trivial code that for the most part
just changes a few constants to update them for spec changes.

https://github.com/mooz/js2-mode/pull/101
https://github.com/mooz/js2-mode/pull/100

(add .diff at the end of each URL to see the plain diff)

Unfortunately, I accidentally complicated #101 by also suggesting a
trivial code transformation: extracting one statement into a function,
which the reporter has promptly performed.

On the face of it, both patches exceed 15 lines in total, but I hesitate
to ask for CA over something this simple (this would also mean delaying
the next merge from Git to elpa for however long that takes).

Any suggestions?

Re: Any exceptions for the 15-line rule?

[Barry OReilly]

[-- Attachment #1: Type: text/plain, Size: 316 bytes --]

> Unfortunately, I accidentally complicated #101 by also suggesting a
> trivial code transformation: extracting one statement into a function,
> which the reporter has promptly performed.

Are you counting the lines that were moved or copied without modification?
The person would have no authorship claim on those.

[-- Attachment #2: Type: text/html, Size: 348 bytes --]

Re: Any exceptions for the 15-line rule?

[Glenn Morris]

Dmitry Gutov wrote:

> https://github.com/mooz/js2-mode/pull/101
> https://github.com/mooz/js2-mode/pull/100
>
> (add .diff at the end of each URL to see the plain diff)

I think it is fair to call those a tiny change; assuming there is
nothing else by the same author.

The 15 lines thing is not totally literal.
A mechanical s/foo/bar on a 100 lines is still a tiny change.

> On the face of it, both patches exceed 15 lines in total, but I hesitate
> to ask for CA over something this simple (this would also mean delaying
> the next merge from Git to elpa for however long that takes).

But let's not make copyright assignment out to be something daunting, or
to be avoided. It's pretty straightforward, very much so if the person
is in the USA. And if it causes a delay, well that's just how it is.

Re: Any exceptions for the 15-line rule?

[Dmitry Gutov]

On 01.05.2013 20:00, Glenn Morris wrote:
> Dmitry Gutov wrote:
>
>> https://github.com/mooz/js2-mode/pull/101
>> https://github.com/mooz/js2-mode/pull/100
>>
>> (add .diff at the end of each URL to see the plain diff)
>
> I think it is fair to call those a tiny change; assuming there is
> nothing else by the same author.
>
> The 15 lines thing is not totally literal.
> A mechanical s/foo/bar on a 100 lines is still a tiny change.

Thank you. I don't think I've seen a good definition of "tiny change" 
anywhere, and there's no s/foo/bar in either of these patches, but if 
you suggest to use my own judgment, that's fine by me.

>> On the face of it, both patches exceed 15 lines in total, but I hesitate
>> to ask for CA over something this simple (this would also mean delaying
>> the next merge from Git to elpa for however long that takes).
>
> But let's not make copyright assignment out to be something daunting, or
> to be avoided. It's pretty straightforward, very much so if the person
> is in the USA. And if it causes a delay, well that's just how it is.

Sure, I'll ask for CA from anyone with sizable and/or repeated 
contributions. Requesting it for small one-off patches feels 
counter-productive, though.

Re: Any exceptions for the 15-line rule?

[Glenn Morris]

Dmitry Gutov wrote:

> Thank you. I don't think I've seen a good definition of "tiny change"
> anywhere, and there's no s/foo/bar in either of these patches, but if
> you suggest to use my own judgment, that's fine by me.

s/foo/bar was just an example.

You need to look at the change and ask, "conceptually, how big is this"?
Deleting says 15 lines of common code from two places and turning it
into a separate function that those two places call only "counts" as a
few lines IMO (the function definition part, the function body is the
same code as before, just moved around).

If in doubt, err on the side of getting an assignment. It also makes
life easier when the same person contributes more in future.

Re: Any exceptions for the 15-line rule?

[Dmitry Gutov]

On 01.05.2013 22:06, Glenn Morris wrote:
> Dmitry Gutov wrote:
>
>> Thank you. I don't think I've seen a good definition of "tiny change"
>> anywhere, and there's no s/foo/bar in either of these patches, but if
>> you suggest to use my own judgment, that's fine by me.
>
> s/foo/bar was just an example.
>
> You need to look at the change and ask, "conceptually, how big is this"?
> Deleting says 15 lines of common code from two places and turning it
> into a separate function that those two places call only "counts" as a
> few lines IMO (the function definition part, the function body is the
> same code as before, just moved around).

I see, that's good.
The two function invocations should probably also count, as +1 or +2.

What about the second patch?

https://github.com/mooz/js2-mode/pull/100.diff

Should it be counted as just +3 (for the added docstring), or does the 
tweaked list of identifiers also contribute? I'm thinking no, because 
the change only removes elements from the list.

What if the diff was adding new elements (like the reverse of this diff 
would do)? Would I be able to discount those lines then, on the basis 
that this is just copying information from the specification?

Re: Any exceptions for the 15-line rule?

[Glenn Morris]

Dmitry Gutov wrote:

> https://github.com/mooz/js2-mode/pull/100.diff

Well, the list of reserved-words could easily have been written on a
couple of lines. It's just spread out into multiple lines due to
formatting. The real change there is just the removal of, what, ~ 10
words? Plus the doc string, total change ~ 4-5 lines IMO.

> What if the diff was adding new elements (like the reverse of this
> diff would do)? Would I be able to discount those lines then, on the
> basis that this is just copying information from the specification?

I don't really know. It's true that in the past we have been told that
information "forced by the interface" is not subject to copyright.
I'd err on the side of counting such lines though.

Re: Any exceptions for the 15-line rule?

[Dmitry Gutov]

On 27.04.2013 8:41, Stefan Monnier wrote:
>> The latter is licensed under GPLv3+, but I'm pretty sure the author
>> (brianjcj AT gmail, not sure what's his full name) has not signed the
>> CA. And I'm not wild about the idea of waiting several months to add the
>> feature (that is, if the author even agrees to sign the CA).
>
> The change includes
>
>     -    (with-temp-buffer
>     +        (buf (get-buffer-create "*clang-output*"))
>     +    (with-current-buffer buf (erase-buffer))
>     +    (with-current-buffer buf
>
> Which seems like it's making the code worse rather than better.
> If you undo this undesirable part of the patch, it'll be closer to
> the acceptable limit.

So, I've merged the patch into Git repo with some modifications, but 
looks like the above change is perfectly justified: the new code uses 
`call-process-region', which takes the region from the current buffer, 
so using it with `with-temp-buffer' doesn't seem possible.

Re: Any exceptions for the 15-line rule?

[Stefan Monnier]

>> -    (with-temp-buffer
>> +        (buf (get-buffer-create "*clang-output*"))
>> +    (with-current-buffer buf (erase-buffer))
>> +    (with-current-buffer buf
> So, I've merged the patch into Git repo with some modifications, but looks
> like the above change is perfectly justified: the new code uses
> call-process-region', which takes the region from the current buffer, so
> using it with `with-temp-buffer' doesn't seem possible.

I must be missing something because I don't see why that
changes anything.



        Stefan

Re: Any exceptions for the 15-line rule?

[Dmitry Gutov]

On 05.05.2013 9:02, Stefan Monnier wrote:
>>> -    (with-temp-buffer
>>> +        (buf (get-buffer-create "*clang-output*"))
>>> +    (with-current-buffer buf (erase-buffer))
>>> +    (with-current-buffer buf
>> So, I've merged the patch into Git repo with some modifications, but looks
>> like the above change is perfectly justified: the new code uses
>> call-process-region', which takes the region from the current buffer, so
>> using it with `with-temp-buffer' doesn't seem possible.
>
> I must be missing something because I don't see why that
> changes anything.

Before, we were just calling `call-process', which uses one buffer, for 
stdout. So we created a temp buffer, collected program output in it, 
parsed that into our data structures and the buffer could be discarded.

Now, `call-process-region' needs two buffers, for stdin and stdout. That 
means that the current buffer has to be the buffer the user's editing, 
or at least contain its contents.

So we could, I guess, save (buffer-string) to a local var, switch to a 
temp buffer, insert it and then call `call-process-region' with non-nil 
DELETE argument (or save the current point value to start parsing output 
from it), but this may be slow for large files (not sure), and using a 
separate names buffer for output is more natural.

And using `with-temp-buffer' to create a non-current buffer (for output) 
is trickier: save current buffer to a local var, `with-temp-buffer', 
save *that* buffer in another `let', use `with-current-buffer' with the 
first var, and then pass the second var to `call-process-region'.

Re: Any exceptions for the 15-line rule?

[Stefan Monnier]

> And using `with-temp-buffer' to create a non-current buffer (for output) is
> trickier: save current buffer to a local var, `with-temp-buffer', save
> *that* buffer in another `let', use `with-current-buffer' with the first
> var, and then pass the second var to `call-process-region'.

I see, that makes sense now, thank you.


        Stefan