Hi, adaptive-wrap[1] implements smart line-wrapping with wrap-prefix. I'd like to proxy-maintain this package. Please see PR[2] on github. [1] https://elpa.gnu.org/packages/adaptive-wrap.html [2] https://github.com/gentoo/gentoo/pull/2370
Two points: - The elisp file has single autoloaded function, which IMHO doesn't merit the overhead of an autoload file. Put this directly into the site-init file instead. (And then the custom src_compile could be dropped from the ebuild.) - Use . or "${WORKDIR}" as destination for cp in src_unpack, not "${S}". Rationale: Usually ${S} will be available only after unpacking (here it doesn't fail because it is equal to WORKDIR, but usage of S in the unpack phase is still unsystematic.) Otherwise, looks good to me.
(In reply to Ulrich Müller from comment #1) > Two points: Thanks for fast response! > - The elisp file has single autoloaded function, which IMHO doesn't merit > the overhead of an autoload file. Put this directly into the site-init file > instead. (And then the custom src_compile could be dropped from the ebuild.) While this doesn't matter in this trivial case, I don't agree in general: why duplicate even a small piece of code from upstream when it's not necessary? I don't believe the overhead of additional file actually matters for user. In other hand, maintainer can easily forget to update autoloads if next release will introduce a new one or e.g. fix description. > - Use . or "${WORKDIR}" as destination for cp in src_unpack, not "${S}". > Rationale: Usually ${S} will be available only after unpacking (here it > doesn't fail because it is equal to WORKDIR, but usage of S in the unpack > phase is still unsystematic.) Got it, fixed. > Otherwise, looks good to me.
(In reply to Victor Gaydov from comment #2) > While this doesn't matter in this trivial case, I don't agree in general: > why duplicate even a small piece of code from upstream when it's not > necessary? I don't believe the overhead of additional file actually matters > for user. It's for the same reason that we combine all small site-init files into a single site-gentoo.el file, namely efficiency. If every other Emacs package starts loading their autoload files, it will make a measurable difference on Emacs' startup time. > In other hand, maintainer can easily forget to update autoloads if next > release will introduce a new one or e.g. fix description. That's rather unlikely for a small package like this.
(In reply to Ulrich Müller from comment #3) > (In reply to Victor Gaydov from comment #2) > > While this doesn't matter in this trivial case, I don't agree in general: > > why duplicate even a small piece of code from upstream when it's not > > necessary? I don't believe the overhead of additional file actually matters > > for user. > > It's for the same reason that we combine all small site-init files into a > single site-gentoo.el file, namely efficiency. If every other Emacs package > starts loading their autoload files, it will make a measurable difference on > Emacs' startup time. > > > In other hand, maintainer can easily forget to update autoloads if next > > release will introduce a new one or e.g. fix description. > > That's rather unlikely for a small package like this. Well, this makes sense. Please see updated PR. I've also fixed filename from "adaptive-wrap-0.5.el" to "adaptive-wrap.el".
(In reply to Victor Gaydov from comment #4) > I've also fixed filename from "adaptive-wrap-0.5.el" to "adaptive-wrap.el". Yeah, that one I had missed. Also, I would prefer an explicit ${P}.el in src_unpack (to make it immediately clear that cp is operating on a single file, without having to look up what A or SRC_URI is), and no quoting unless necessary: cp "${DISTDIR}"/${P}.el ${PN}.el || die
(In reply to Ulrich Müller from comment #5) > (In reply to Victor Gaydov from comment #4) > > I've also fixed filename from "adaptive-wrap-0.5.el" to "adaptive-wrap.el". > > Yeah, that one I had missed. > > Also, I would prefer an explicit ${P}.el in src_unpack (to make it > immediately clear that cp is operating on a single file, without having to > look up what A or SRC_URI is), and no quoting unless necessary: > > cp "${DISTDIR}"/${P}.el ${PN}.el || die Done.
Thank you for the contribution! @proxy-maint: Good to go.
@maintainer @ulm pushed, thanks commit d8da69633924e697e9f69524d92b1e4b5620c994 Author: Victor Gaydov <victor@enise.org> Date: Tue Sep 20 13:56:26 2016 +0300 app-emacs/adaptive-wrap: new package adaptive-wrap implements smart line-wrapping with wrap-prefix. Gentoo-Bug: https://bugs.gentoo.org/show_bug.cgi?id=594502 Closes: https://github.com/gentoo/gentoo/pull/2370 Signed-off-by: Yixun Lan <dlan@gentoo.org>
