Fixing issue #19 #30
Fixing issue #19 #30
Conversation
lib/progress_bar.ex
Outdated
| list | ||
| end | ||
|
|
||
| def from_steam(list, opts \\ []) do |
henrik
left a comment
henrik
left a comment
There was a problem hiding this comment.
Hi! Thanks so much – wrote some comments.
Would you be OK also updating the README to document the new functionality? I might do another round on that after but it would be great to have as a starting point.
test/enum_test.exs
Outdated
|
|
||
| import ExUnit.CaptureIO | ||
|
|
||
| test "it should work with enums" do |
There was a problem hiding this comment.
Could you please write these without "should" to match other tests, e.g. "it works …"?
test/enum_test.exs
Outdated
| import ExUnit.CaptureIO | ||
|
|
||
| test "it should work with enums" do | ||
| capture_io(fn -> |
There was a problem hiding this comment.
Could you make the tests cover more of actual behaviour? I feel like they should cover:
- That it outputs a progress bar (ideally that it does so bit by bit as it iterates, but at least checking for the final state after the iteration).
- That it calls the callback function once per iteration.
- What the function returns (we do on line 19 below but not here).
| end | ||
|
|
||
| def from_steam(list, opts \\ []) do | ||
| total = Enum.count(list) |
There was a problem hiding this comment.
My Elixir is rusty (haven't written much in the past few years) and I never were super familiar with streams.
Is it reasonable to execute the stream here by counting the length? It was suggested in #19 that the length could be passed in, but I'm not sure in what situations you might know the length and also have a stream that you don't want to execute here. The user will need to execute it again after the map/each below to get any output.
cc @kelvinst if you remember or still have any specific use case around this where this detail would matter.
There was a problem hiding this comment.
Well thought. I can't think of a case where one would have the length of a stream before actually going through it. Maybe one would already go through the stream of another operation and already have its length, but then it does not make sense to attach a progress bar after that again, IDK.
Anyway, I think it's ok to get the length this way, we might just want to warn people that the Stream is going to be traversed to get its final length and print the %
There was a problem hiding this comment.
I will add this message to readme
|
@henrik thank you for you review, i added changes you requested for! |
|
@henrik kind reminder |
|
Hi, sorry for letting this languish forever. I've not done much Elixir lately and I'm not actively maintaining this library. I've now updated the README to say it's not maintained. If anyone wants to take it over (e.g. the Hex package), let me know. |
3 participants
No description provided.