Re: yasnippet commits review
Hello Nicholas,
Le mercredi 4 janvier 2023 à 13:13, Nicholas D Steeves
<sten@debian.org> a écrit :
ca55bc8: Nice find, and fix, thank you! (I haven't tested it
yet, but
assume you have, that it's good, and that it will pass when I
test it)
I have, and I have checked that the generated HTML documentation
is the same as in currently installed versions of yasnippet (from
what I can tell, only the order of the documented symbols in the
documentation changes, which could be related to changes in the
way org export works). If you think the patch is good, I'll
forward it upstream and explain my thinking.
2ff39eb: Nitpick: If you patch fixes the incompatibility, please
say so
:) Also, it may be nice to say that it's related to #1020143 if
that's
the case.
I don't think my patch has anything to do with this problem, which
must have stopped being applicable at some point in the past. I
just could not reproduce it anymore.
8aa6556: Thank you for noting "no changes required" because this
item
requires a human to check for compliance :)
I checked against the policy checklist.
7c4ae4a: Oh wow, this looks like something that may may have
been out of
date for years. Thanks!
Cheers.
b9ccd2b: What's the purpose of this digression from the way
dh_elpa does
things? It looks like it introduces potential indeterminacy.
At a
minimum, an explanation needs to be provided.
I just modeled the override according to what I did in my other
packages (see vertico or consult, for instance), for which I had
no objection. But it turns out that dh_elpa does "emacs -Q -L
/path/to/needed/elpa/src/" rather than "emacs -q". The second one
seemed simpler, but they are not entirely equivalent, the debian
site-files do a little bit more than just adding directories to
the load-path. I will remove this commit.
0a76135: I will not sponsor due to this action, because it's not
ok
to disable all 91 tests, when only 7 are failing
https://ci.debian.net/data/autopkgtest/unstable/amd64/y/yasnippet/28997908/log.gz
Feel skip these seven tests using another method, but please say
why
this is the correct action.
The thinking was the following :
The tests' failing has nothing to do with us, a simple "rake
tests" on the upstream repo fails the same, and upstream seems
unconcerned about this. In fact, there is a PR upstream (#1125)
that solves some of the failing tests that hasn't been merged in
more than a year.
I would have liked the failing tests not to prevent the build from
succeeding, but still be able to see that there are some
failing. By disabling dh-elpa-test, we make the build succeed, but
autopkgtest still runs after the build, and the failure can still
be visible on ci.debian.net (which is good). If we skip the tests,
we're making them disappear from the build and from autopkgtest.
I have neither the time nor the inclination to investigate the
failing tests. So I can either :
- skip the tests
- make something like :
override_dh_elpa_test:
-dh_elpa_test
This would still run the tests, but the fact that they fail would
not make the build fail.
Tell me what you prefer, I'll rebase accordingly on the temp
branch.
Best, and thank you for the review,
Aymeric
Reply to: