Re: [NonGNU ELPA] New package: project-store

Lin Jian <[email protected]> writes:

> Thanks for your review.
>
> Philip Kaludercic <[email protected]> writes:
>
>>> It is a project.el backend for Nix store.  Guix users may also find it
>>> useful.  The full README is available at the end of this email.
>>
>> Can you clarify what this means?  From what I understand, this is
>> suppose to be a backend that interprets every directory underneath
>> /nix/store as a project, right?
>
> Yes, as said by the second sentence of README.  There are also some
> example directories and their corresponding projects instances in the
> ERT test `project-store-try' in project-store-tests.el.
>
>> A few comments:
>>
>> - I wouldn't refer to README in Commentary, it would be nice to have a
>>   brief summary of the package in that place.
>
> I am not a big fan of duplicating things and I prefer a single source of
> truth.  Referring to README or `describe-package' in Commentary achieves
> that goal and, at the same time, covers three main use cases well:
>
> - When users read Commentary in Emacs, they can easily run
>   `describe-package' to get more documentation, if they are not
>   satisfied with the brief description at the first line of
>   project-store.el.

Only if you have installed the package.

> - When users read the ELPA homepage, it includes README.

This is true, by default at least.

> > - When users read Commentary from the source code hosting platform, they
>   have access to README.

I would still argue that the README and the Commentary have two
different target audiences: The former is a general introduction to the
repository and usually contains information about how to contribute,
what the layout of the project is, how to build it, etc. while the
Commentary is specifically targeted at the user and reader of the Elisp
script.  If I do a M-x find-feature to open a file, it is annoying to
find an indirection.  So for me this wouldn't be a duplication at all,
but two different expositions.

> That being said, I am open to add some duplication in Commentary if
> there are other common use cases which justify the duplication.

I am not saying you have to, I would just recommend as it is
conventional and IMO useful to have a standalone commentary for the
script.

>> - If you want to suggest compatibility with Guix, would it make sense to
>>   have the type of `project-store-dir' also include a constant for
>>   /gnu/store/?
>
> When I write this package, I have Nix store in mind.  I do not use Guix.
> (Also, unlike Nix, I did not find doc about Guix store specification.)
> So I do not plan to keep this package working with Guix, if Guix store
> diverges from Nix store in the future.  Note that I also do not object
> to fixing Guix compatibility.  Patch is welcome.
>
> I am careful with the wording about Guix in the documentation and code.
> My intention is to convey only that currently project-store *happens* to
> also work with Guix store.  So I will not mention Guix in the type of
> `project-store-dir'.

My understanding is that this is unlikely to happen, but I also don't
know for sure.

>>   What if someone uses both Guix and Nix (or other forks)?
>
> This is a use case that I did not think of.  I do not object to
> supporting it.  Patch is welcome.

IIUC this would basically amount to having a list of directories that
could serve as stores?

>> - Do you really want to include project-store-benchmark.el and
>>   project-store-tests.el in the package, or should they be excluded in
>>   .elpaignore?
>
> I want to include project-store-tests.el in the package, which can be
> useful for downstream distributions to run tests during their build
> process.  I know Guix already runs tests during Emacs lisp package
> build.  I plan to do so for NixOS.

Oh, do Nix/Guix use our tarballs, instead of building directory from
source?

> For project-store-benchmark.el, I am not quite sure if it should be
> included in the package.  Maybe it makes it easier to hack this package?
> Is there any disadvantage of including it?

The only disadvantages I can think of are file size and noise.

> While we are at it, should license files be excluded via .elpaignore?  I
> am really confused by this.  I see about 33% NonGNU ELPA packages ignore
> LICENSE or COPYING via :ignored-files in elpa-packages.  Maybe more
> packages ignore license files in their .elpaignore file.  However, there
> are also ELPA packages that include license files, such as popon,
> corfu-terminal, auctex and org.  My gut feeling is that license files
> *should* be distributed together.  Documenting answer to this question
> somewhere would help package authors a lot, I think.

I'd have to check, but for packages on GNU ELPA which are considered to
legally be part of Emacs, it ought not be necessary to distribute the
GPL additionally since you have already received a copy along with
Emacs.  But I am not sure if this argument also applies to NonGNU ELPA.

>> - I think you can simplify `project-store--try-without-cache' using
>>   `locate-dominating-file', something like
>>
>>     (locate-dominating-file dir (lambda (dir*) (string= (file-name-parent-directory dir*) project-store-dir)))
>
> At first glance, this implementation seems to involve more function
> applications than the while loop in current implementation.  In
> addition, there is `string-match' in the loop of
> `locate-dominating-file'.  So presumably this implementation is slower.
> I benchmarked it: this implementation uses 2.9x wall-clock time of the
> current implementation for `project-store--try-without-cache'; no time
> usage change in other functions.
>
> For style, I do not have a strong opinion about these two options.  They
> are equally readable to me.  But for other people, maybe this
> implementation is more readable.
>
> To sum up, I slightly prefer the current implementation.

Then that is fine.

>> - Do you need to use the not-found constant in `project-store-try'?  Or
>>   can't you just simplify the entire expression to the
>>   `with-memoization' block without `or'?
>
> Since `with-memoization' cannot distinguish nil and "no value yet", to
> cache cases where DIR is not in a store project, we have to use
> something other than nil (here, we use 'not-found) in the `or' block.

Ah yes, I forgot about that.  In that case I would add a comment to
avoid confusing future readers.

>> - Why is `project-store-p' autoloaded?
>
> `project-store-p' is added to `project-list-exclude' which is used in
> the *autoloaded* interactive command `project-remember-project'.  So it
> is possible that `project-store-p' is called before the other autoloaded
> function `project-store-try'.  So we should autoload it.

Here again a comment would be useful.

>> - How much of a difference does the caching make?
>
> There is no noticeable difference in my use case: moving point around in
> a buffer with breadcrumb-1.0.1 enabled.  However, there is a difference
> in profiling.  (Of course, there is also a difference in benchmarks.)
>
> Even with no noticeable difference, I decide to go caching since it
> optimizes the above common use case very well, with no obvious
> disadvantage.
>
> Here is the profiling results of the same point movement in the same
> buffer with breadcrumb-1.0.1, with and without cache in project-store:
>
> With cache:
>
>          710  88% - redisplay_internal (C function)
>          185  23%  - eval
>          159  19%   - breadcrumb--header-line
>          147  18%    - funcall
>          146  18%     - breadcrumb-project-crumbs
>          127  15%      - breadcrumb--project-crumbs-1
>           66   8%       + file-relative-name
>           35   4%         breadcrumb--format-project-node
>            7   0%       + file-name-base
>            6   0%         split-string
>            4   0%         directory-file-name
>            1   0%       + project-root
>            1   0%         propertize
>            1   0%         project-current
>           17   2%        breadcrumb--summarize
>
>
> Without cache:
>
>          931  87% - redisplay_internal (C function)
>          362  34%  - eval
>          321  30%   - breadcrumb--header-line
>          301  28%    - funcall
>          269  25%     - breadcrumb-project-crumbs
>          256  24%      - breadcrumb--project-crumbs-1
>          105   9%       - project-current
>          104   9%        - project--find-in-directory
>          104   9%         - run-hook-with-args-until-success
>          104   9%          + project-store-try
>           77   7%       + file-relative-name
>           43   4%         breadcrumb--format-project-node

Unless I am misreading this, it seems to be less of a difference than I
expected?  Either way, my main point here was to start the discussion of
possibly up streaming the caching generic code to project.el itself.

But that shouldn't stand in the way of adding the package itself to
NonGNU ELPA.  I'll leave some more time for further comments, but IMO
everything ought to be fine now :)