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

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.
- When users read the ELPA homepage, it includes README.
- When users read Commentary from the source code hosting platform, they
  have access to README.

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

> - 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'.

>   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.

> - 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.

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?

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 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.

> - 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.

> - 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.

> - 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:

--8<---------------cut here---------------start------------->8---
         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
--8<---------------cut here---------------end--------------->8---

Without cache:

--8<---------------cut here---------------start------------->8---
         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
--8<---------------cut here---------------end--------------->8---