unofficial mirror of bug-gnu-emacs@gnu.org 
 help / color / mirror / code / Atom feed
From: "João Távora" <joaotavora@gmail.com>
To: Damien Cassou <damien@cassou.me>
Cc: 38284@debbugs.gnu.org, Nicolas Petton <nicolas@petton.fr>
Subject: bug#38284: 27.0.50; [PATCH] Make auth-source-pass-search understand port lists
Date: Fri, 22 Nov 2019 09:35:38 +0000	[thread overview]
Message-ID: <87lfs8b6dx.fsf@gmail.com> (raw)
In-Reply-To: <87lfs8thwv.fsf@cassou.me> (Damien Cassou's message of "Fri, 22 Nov 2019 09:49:20 +0100")

Damien Cassou <damien@cassou.me> writes:

> João Távora <joaotavora@gmail.com> writes:
>>> can you please rename "d" to "domain"?
>>
>> ok.  I do call your attention that it was already the
>> single letter n there
>
> yes I saw. This should never have been merged like that. Thank you for
> improving the code.

I think the single letter idiom is fine there.  It was just the wrong
letter.  By the way, I've left the p for the port, because calling it
"port", while it would work, would seriously confuse a reader.

>>> Can you please add a unit test covering this new use-case?
>>
>> No. This is too much work for such a trivial change
>
> I care a lot about the automated testing of the code I write.

Certainly, I care a lot, too.  I don't write tests for these changes out
of principle, not out of lazyness.  Most, if not all, the projects I
manage have automated tests.

> I won't try to convince you though. Can you please merge the attached
> patch with yours?

No, but you can do that, because it's your work (I can push it for you
though).  Anyway, now I read the test you wrote, I agree it's a good
test.  You are testing auth-source-pass-match-entry-p, much higher up
than auth-source-pass--generate-entry-suffixes, the function I changed.

Of course, only someone who was involved in the design would be able to
confidently place the tests at that correct level, the finding of which
is the most difficult part.

My advice and personal opinion is to later use this and more such tests
to perhaps redesign/cleanup the auth-source-pass.el library, which seems
needlessly complicated in the little stuff like the function I touched.
(to be fair it wasn't much helped by the style of the auth-source.el
parent library.)

João





  reply	other threads:[~2019-11-22  9:35 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-11-20  0:20 bug#38284: 27.0.50; [PATCH] Make auth-source-pass-search understand port lists João Távora
2019-11-21 13:47 ` Lars Ingebrigtsen
2019-11-21 18:27 ` Damien Cassou
2019-11-21 19:06   ` João Távora
2019-11-22  8:49     ` Damien Cassou
2019-11-22  9:35       ` João Távora [this message]
2020-01-20 18:54         ` Stefan Kangas
2020-01-21 19:25           ` Damien Cassou
2020-01-22  8:02             ` Stefan Kangas
2020-01-22  9:22               ` Damien Cassou

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: https://www.gnu.org/software/emacs/

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87lfs8b6dx.fsf@gmail.com \
    --to=joaotavora@gmail.com \
    --cc=38284@debbugs.gnu.org \
    --cc=damien@cassou.me \
    --cc=nicolas@petton.fr \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).