Re: NonGNU ELPA: Conditions for accepting a potential new package 'rmsbolt' ?

[Jay Kamat <jaygkamat@gmail.com>]

Philip Kaludercic <philipk@posteo.net> writes:

Might take me a while to fully review and test all the changes, but they look 
good, so once I do so, I'll include them!

> +(defcustom rmsbolt-mode-lighter " RMS🗲"	;could the unicode charachter be 
> removed?  it can unnecessarily resize the mode line.

Sure - I can remove it. I'll do it after I apply your patch generally.

> -(defun rmsbolt--convert-file-name-to-system-type (file-name)
> +(defun rmsbolt--convert-file-name-to-system-type (file-name) ;perhaps use 
> `convert-standard-filename'?

Right now, a contributor is trying to improve this area/enable tramp support - 
so I'd like to keep this as stable as possible while they do so. But this is a 
great suggestion - I'll let them know about this function and hopefully it 
will help them in their change as well!

> -(defmacro rmsbolt--set-local (var val)
> +(defmacro rmsbolt--set-local (var val)	;`setq-local' is avaliable from 
> 24.3 onwards

I'll make this change after I get this in, thanks!

> -          (cmd (mapconcat
> +          (cmd (mapconcat		;you have `string-join' in subr-x
>                  #'identity
>                  (list compiler "-g" "-output-s" src-filename "-of" 
>                  output-filename)
>                  " ")))

Not sure why I missed this - I'll fix this after this patch as well - thanks!

> -                (setq rmsbolt--temp-dir nil)))))
> +                (setq rmsbolt--temp-dir nil))))) ;why is this necessary 
> while killing emacs?

This is mostly me being very paranoid - as this function could delete random 
directories if misconfigured/in error, I want to disable it as soon as 
possible.

>
>  ;;;;; Starter Definitions
>
>  ;; IIUC, this "starter" business is not a necessary part of RMSBolt, but is
>  ;; a way to provide sample files with which users can try out RMSBolt.
>
> -(defvar rmsbolt-starter-files
> +(defvar rmsbolt-starter-files		;should this be part of a secondary 
> file that isn't loaded by default?

Maybe - I think I would load it by default though - as this is the easiest way 
for someone to try out the package. In my personal usage I almost always start 
from one of these files and add my changes. I should update the comment above 
with that information as well.

Thanks! I just want to be doubly sure of all the changes before I commit them, 
I'll try to do that testing tomorrow - I'm a bit busy today. I'll also try to 
fix any remaining checkdoc errors.

-Jay