-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
print progress in lein release #2605
print progress in lein release #2605
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.
Thanks for looking at this, I've made some suggestions.
src/leiningen/release.clj
Outdated
(main/resolve-and-apply current-project task)))))) | ||
(let [all-tasks (:release-tasks project) | ||
total-task-count (count all-tasks)] | ||
(doseq [[i task] (zipmap (range total-task-count) all-tasks)] |
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 don't think zipmap
is the right function for this, it returns a map, not a sequence. Below 8 entries everything will appear to work, but above 8 the order of the entries in the map is not the same as their insertion. I think map
would work better here.
Side note, zipmap
(and map
) can take a finite and infinite sequence and produce a finite map, e.g. (zipmap (range) all-tasks)
would also work.
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 think we might want to start counting at 1 rather than 0 here? When I see 2/3, I think that that is the second task of the three, when it's actually the last one. That would also mean we could avoid having a final "Completed lein release." log message at the end.
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 don't think zipmap is the right function for this, it returns a map, not a sequence.
I should use (map vector (range) release-tasks)
here.
I think we might want to start counting at 1 rather than 0 here?
I'm not sure about this. I realized Chrome text search(Ctrl+F) starts from 1, so I'll take your suggestion.
src/leiningen/release.clj
Outdated
(let [current-project (project/init-project (project/read))] | ||
(main/resolve-and-apply current-project task)))))) | ||
(let [all-tasks (:release-tasks project) | ||
total-task-count (count all-tasks)] |
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.
task-count might be a better name?
total-task-count (count all-tasks)] | |
task-count (count all-tasks)] |
src/leiningen/release.clj
Outdated
(doseq [task (:release-tasks project)] | ||
(let [current-project (project/init-project (project/read))] | ||
(main/resolve-and-apply current-project task)))))) | ||
(let [all-tasks (:release-tasks project) |
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.
(let [all-tasks (:release-tasks project) | |
(let [release-tasks (:release-tasks project) |
src/leiningen/release.clj
Outdated
(let [all-tasks (:release-tasks project) | ||
total-task-count (count all-tasks)] | ||
(doseq [[i task] (zipmap (range total-task-count) all-tasks)] | ||
(apply main/info (concat ["[" i "/" total-task-count "] Executing lein"] task)) |
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 prefer "running" to "executing" but I think this is mostly a matter of taste.
(apply main/info (concat ["[" i "/" total-task-count "] Executing lein"] task)) | |
(apply main/info (concat ["[" i "/" total-task-count "] Running lein"] task)) |
b918907
to
db0a5e7
Compare
@danielcompton I like all your suggestions.
CI failed but |
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.
This is looking pretty close. I’ll defer to others on whether they’d prefer to see a different format for the log messages. I don’t know if there is a more “Leiningen style” logging format?
src/leiningen/release.clj
Outdated
(let [release-tasks (:release-tasks project) | ||
task-count (count release-tasks)] | ||
(doseq [[i task] (map vector (range 1 (inc task-count)) release-tasks)] | ||
(apply main/info (concat ["[" i "/" task-count "] Running lein"] task)) |
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.
This might be clearer using format, or possibly just removing the concat and wrapping collection, I don’t think it’s necessary?
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 agree.
db0a5e7
to
7e1f7bb
Compare
7e1f7bb
to
8f0f0b0
Compare
Thanks; this looks great. I can't think of any other comparable output to try to make it more consistent with, so I think we should go with this. I'm looking into the CI failure but it's unrelated to your change. |
justification
This make it easier to rollback release-tasks or complete them by hand.
#2402
lein release
works like this with this patch.