search

mfontanini / presenterm

A markdown terminal slideshow tool
https://mfontanini.github.io/presenterm/
BSD 2-Clause "Simplified" License
1.19k stars 29 forks source link

feat(arg): demo arg to get `example` from repo and present #149

Closed pwnwriter closed 4 months ago

pwnwriter commented 8 months ago

As you've already mentioned, integrating the demo assets directly into the binary itself might be a bit excessive. So, To address #125, I have added an argument that retrieves the demo assets to /tmp/presenterm-demo directory and presents it.

POC:

recording.webm

mfontanini commented 8 months ago

I appreciate the PR, but I don't think this is the way to go. This is adding a whole bunch of dependencies that are unnecessary for the tool to run, only to save the people who want to run the demo presentation from running a single extra command.

I think this should instead be a note in the docs that says "if you want to run the demo presentation, run git clone ...., then presenterm ..., etc".

pwnwriter commented 8 months ago

There is only one dependency for this PR: reqwest ; is used to fetch assets. Apart from that, there is no dependency cycle.

While I acknowledge there might be alternative approaches, I submitted this pull request to resolve the issue. The decision to accept or reject is yours, and I'm fine with whatever you choose.

mfontanini commented 8 months ago

There's roughly 50 transitive dependencies being pulled in, check Cargo.lock. Some of them can probably be avoided (e.g. futures related ones) but most of them probably not. Do you think running git clone is too much hassle for people? I presume given this is a terminal based tool, people who use it should be okay with that.

pwnwriter commented 8 months ago

Well, Technically, there might be more than 50 dependencies, but once compiled, there are no actual dependencies, and the binary itself doesn't undergo any size changes. Compiled with Optimized profile*

Running git isn't a hassle but I thought it'd be nice to have something like demo built inside the tool itself. 😅

mfontanini commented 8 months ago

This matters when you're developing the tool. Build times are slower, there's more dependencies that need to be updated, etc. Bloat matters; you shouldn't pull in dependencies just because the final binary doesn't show noticeable size changes.

pwnwriter commented 8 months ago

It's okay, I think let it be for now and if someday we need any http crate , we can think about it .

mfontanini commented 4 months ago

Closing this. We can reopen if it makes sense to add it at some other time.