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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this 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) |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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?
@J0sueTM ping :-) |
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. |
There was a problem hiding this 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) |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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!
(let* ((files (directory-files-recursively directory "\\.clj[cs]?$")) | ||
(file-count (length files)) | ||
(i 0)) | ||
(mapcar (lambda (file) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)))))) |
There was a problem hiding this comment.
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
.
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 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) |
@J0sueTM ping :-) |
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! |
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) |
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):
eldev test
)eldev lint
) which is based onelisp-lint
and includescheckdoc
, check-declare, packaging metadata, indentation, and trailing whitespace checks.Thanks!
If you're just starting out to hack on CIDER you might find this section of its
manual extremely useful.