Skip to content

feat: add progress_path helper with basic tests#232

Open
Gitkbc wants to merge 1 commit into
lunarmodules:mainfrom
Gitkbc:feat/progress-path
Open

feat: add progress_path helper with basic tests#232
Gitkbc wants to merge 1 commit into
lunarmodules:mainfrom
Gitkbc:feat/progress-path

Conversation

@Gitkbc

@Gitkbc Gitkbc commented Mar 19, 2026

Copy link
Copy Markdown
Contributor

Adds a simple progress_path helper to render a left-to-right progress animation.

Includes basic tests to verify:

  • rendering output format
  • percentage clamping between 0 and 100

Kept the implementation minimal (based on feedback from #226) & consistent with existing progress utilities.

@Gitkbc Gitkbc force-pushed the feat/progress-path branch from eb1fdcd to 187b6d6 Compare March 19, 2026 19:20
Comment thread spec/13-progress_spec.lua Outdated
Comment thread src/terminal/progress.lua Outdated

local percent_str = string.format("%3d%%", percent)

return "\27[K" .. bar .. " " .. percent_str

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

these hardcoded ansi sequences will mess with the stacks. ALso should we be displaying percentages? none of the others does that.
Maybe we can make that a higher level progress bar, something in the CLI namespace

@Gitkbc Gitkbc force-pushed the feat/progress-path branch from 187b6d6 to de7dcda Compare April 2, 2026 18:52
@Gitkbc

Gitkbc commented Apr 2, 2026

Copy link
Copy Markdown
Contributor Author

Updated: removed the hardcoded ANSI sequence and percentage display. Fixed whitespace to match the rest of the spec file.

@Tieske Tieske left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I have some doubt on this one. I like the idea, the graphical display.

But my doubts are on the design, it is completely different from the others. Take a look at the 'ticker' one, right below. That does a similar thing, but it generates a sequence that can be used like a sprite (similar to all others)

wdyt?

Comment thread src/terminal/progress.lua Outdated
-- progress_path(40, 20, "📍", "🚙")
--
-- Output example:
-- 📍........🚙 40%

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

still showing percentage in example

Comment thread src/terminal/progress.lua Outdated
Comment on lines +149 to +150
-- @tparam string start_icon icon at the start position
-- @tparam string runner_icon icon representing the runner/progress

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

these 2 are optional, but don't have their defaults documented

@Gitkbc

Gitkbc commented Apr 3, 2026

Copy link
Copy Markdown
Contributor Author

@Tieske Sir Thanks for the review!

My original thinking:
I built progress_path as a simple visual helper that returns a path string (a runner icon moving through dots) which can be used for loading or progress indicators.I completely see your point, though. The current version doesn't match the sprite-sequence pattern used by ticker and the other helpers.

Refactoring it to return a sequence of frames (like ticker) makes it much more consistent and flexible. I am going to go ahead and refactor it in that direction so it matches the architecture.

I'll also clean up the docstring (remove the old percentage example and document defaults) in the same update.

@Gitkbc Gitkbc force-pushed the feat/progress-path branch from de7dcda to 36243c7 Compare April 20, 2026 17:56
@Tieske

Tieske commented Apr 21, 2026

Copy link
Copy Markdown
Member

there seem to be conflicts, can you rebase?

Refactored to return sprite sequence (aligned with ticker/spinner),
removed percent-based logic, and updated docs/tests accordingly.
@Gitkbc Gitkbc force-pushed the feat/progress-path branch from 36243c7 to 7ea2cb2 Compare April 21, 2026 07:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants