Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a visual progress for when evaluating a directory recursively #3615

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

J0sueTM
Copy link

@J0sueTM J0sueTM commented Jan 31, 2024

Recently, since I'm working with a big clj project, I periodically use cider-eval-all-files, and it works fine. The only problem is that I always underestimate the time it takes to compile all the files in a directory, and endup evaluating another directory while the first one is still in progress.

That's what I try to solve with this PR. By adding a visual progress, it becomes easier to know if its OK to continue evaluating other modules of my code that depends on the thing that's currently being done.

BTW, I ran eldev lint, and some errors showed up, but none of them had to do with my changes.


Before submitting the PR make sure the following things have been done (and denote this
by checking the relevant checkboxes):

  • The commits are consistent with our contribution guidelines
  • You've added tests (if possible) to cover your change(s)
  • All tests are passing (eldev test)
  • All code passes the linter (eldev lint) which is based on elisp-lint and includes
  • You've updated the changelog (if adding/changing user-visible functionality)
  • You've updated the user manual (if adding/changing user-visible functionality)

Thanks!

If you're just starting out to hack on CIDER you might find this section of its
manual
extremely useful.

Copy link

@lanjoni lanjoni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@vemv vemv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like a reasonable addition - kudos!

Please simply satisfy the small feedback and ensure there's a green build.

Cheers - V

cider-eval.el Outdated
(total (length files)))
(mapcar (lambda (file)
(cider-load-file file undef-all
(+ (cl-position file files :test #'equal) 1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would seem to me that you could use a simple counter that would be incremented on each iteration

cider-eval.el Outdated
@@ -1782,15 +1782,21 @@ all ns aliases and var mappings from the namespace before reloading it."
callback)))
(message "Loading %s..." filename))))))

(defun cider-load-file (filename &optional undef-all)
(defun cider-load-file (filename &optional undef-all pos total)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've got some concern as to whether this should be in the public API of the command, given that it's needed only by the counter. Perhaps we can introduce a helper function to hide this?

@bbatsov
Copy link
Member

bbatsov commented Feb 28, 2024

@J0sueTM ping :-)

@J0sueTM
Copy link
Author

J0sueTM commented Feb 28, 2024

Thanks for pinging @bbatsov completely forgot about it! My time hasn't been the most generous recently 😅 . I'll be coming with changes in the coming days.

@J0sueTM J0sueTM requested a review from bbatsov March 2, 2024 19:40
cider-eval.el Outdated Show resolved Hide resolved
Copy link
Member

@vemv vemv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for picking up this!

Left a last round, please ensure a green build + no conflicts as well

cider-eval.el Outdated
(directory-files-recursively directory "\\.clj[cs]?$")))
(let* ((files (directory-files-recursively directory "\\.clj[cs]?$"))
(indexed-files (index-files files)))
(mapcar (lambda (indexed-file)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While number-sequence is a nice function, I'd prefer it you used local mutation as it would seem easier to follow here.

An example:

(let* ((files '("a" "b" "c"))
       (i 0))
        (mapcar (lambda (file)
                  (message (format "%s %s" file i))
                  (setq i (inc i)))
                files))

So, you can use the existing mapcar to mutate a local counter, so we have one mapcar instead of two (plus the helper function).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing out. My elisp knowledge is really limited and that helped a lot. Thanks!

@J0sueTM J0sueTM requested a review from vemv March 2, 2024 21:19
(let* ((files (directory-files-recursively directory "\\.clj[cs]?$"))
(file-count (length files))
(i 0))
(mapcar (lambda (file)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better to use mapc instead of mapcar if all you care about are the side-effects.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking again at the code - probably using dolist would be even better than mapc, as it reflects best what we want to do anyways.

@@ -1831,33 +1832,41 @@ all ns aliases and var mappings from the namespace before reloading it."
(file-name-nondirectory filename)
repl
callback)))
(message "Loading %s..." filename))))))
(message (concat "Loading " progress "%s...") filename))))))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should not use concat in message, as you can just use format specifiers like %s, %d, etc. It's similar to Clojure's format.

@bbatsov
Copy link
Member

bbatsov commented Mar 3, 2024

Btw, I still don't like that we're now passing some text around. Might be better to make the message optional or move the progress reporting to the function that deals with many namespaces or to rename the progress param to something like msg, so it's clearer what this is.

Also - for a lot of files some status bar might be a better option (e.g. https://www.gnu.org/software/emacs/manual/html_node/elisp/Progress.html or spinner.el that we're using elsewhere in the code)

@bbatsov
Copy link
Member

bbatsov commented May 7, 2024

@J0sueTM ping :-)

@J0sueTM
Copy link
Author

J0sueTM commented May 7, 2024

hey @bbatsov. I'm probably not able to finish this PR in the way you guys want. Nothing wrong with that, of course, I just don't know much about ELisp or have enough time in order to accomodate your quality requests 😅.

I'm sorry for taking your time. Could you please either close the PR or backlog it so, if on interest of anyone, implement this better later?

Thanks for the great lib. I use it daily and it work flawlessly!

@vemv
Copy link
Member

vemv commented May 7, 2024

It's no issue. Normally in these cases someone from us can pick up the branch and do whatever's needed, preserving attribution to you.

(Will do)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants