Re: [ELPA] New package: cursor-undo

[Philip Kaludercic <philipk@posteo.net>]

路客 <luke.yx.lee@gmail.com> writes:

>>  Before pushing a package directly to elpa.git, you should try to build
>>  it locally.  Currently it fails because the :doc specification links to
>>  a .txt file, which cannot be converted to a Info manual:
>
> I followed the ELPA steps but there is no info about how to build a new
> package locally before pushing into the main git branch external/<package>
> . A "make build/cursor-undo" command sometimes cause a pull from external
> git.  Even in the ELPA `elpa-admin.el' the first TODO is:
>
>   ;; - bug#45345: [elpa-archive] "make build/<package>" should not pull
>   ;;   unconditionally
>
> and I found this is /sometimes/ true.  Sometimes I modified my source code
> and do "make build/cursor-undo" again it will be overwritten back to the
> old one in git external/cursor-undo.  Therefore, the only way is to push
> it back into git and wait a while then rebuild.  However, sometimes it
> won't so it's confusing what the correct steps to perform a pure local
> build really are.  So, let me know if there is a way to build a new
> package locally without push into the external/<package> first.

I have to admit that I am not sure about this, but I can look into it.
In the last few years the recommended workflow has been to use an
external repository that is mirrored into elpa.git, so I guess this
hasn't been an issue that a lot of people have encountered recently.

> But as a hindsight I now learned I could first modify "elpa-packages"
> locally and build but still need to push into external/<package> first.
> Next time I will make sure I do that.
>
> After I managed to build my package I found the ELPA README did not mention
> that the ";;; <package> ends here" is required.

That is documented in (elisp) Library Headers.  It is not a
ELPA-specific rule.

> Some of your comments are followed while some not:
>
>> +;; shouldn't this be a major mode?
>> (defvar cundo-enable-cursor-tracking t) ; global enable flag
>
> Actually this package is not a mode, but it seems it's preferred as Stefan
> also mentioned this to me so I now made it a minor mode.

1+

>>                   ;; unless I am misreading this, `prev-screen-start'
>>                   ;; is being evaluated in the context of the macro,
>>                   ;; /not/ the macro-expansion.  Yet
>>                   ;; `prev-screen-start' is bound and set inside the
>>                   ;; context of the macro-expansion.  This seems to
>>                   ;; trigger the 'Unused lexical variable
>>                   ;; ‘prev-screen-start’' byte-compiler warning
>>                   ;; below.
>
> Yes it was meant to be evaluated instead of being expanded since the
> beginning many years ago, this part of code has been tested for many
> years, I reviewed the generated code again and it's still valid.  I think
> the byte compiler must have mis-interpreted my intention and gave the
> incorrect warnings.  Anyway, before the root cause is found a quick
> mitigation is to (defvar prev-screen-start).

Let's hope so.

>>  +(provide 'cursor-undo)
>
> This was done earlier in the code.

Must have missed it then.  Is there a reason you didn't add it at the
end, as is conventional (and probably useful, if there is an error while
loading the file, that you don't mark it as required)?

>> +;;; cursor-undo.el ends here
>
> This step is a must but the ELPA README did not mention that.  I found
> the local build would fail without it, so it's now added.
>
> The fixed version has been pushed into the git.  Thanks.
>
> Best regards,
> Luke Lee

-- 
	Philip Kaludercic on peregrine