Skip to content
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

feat: replace less with minus #100

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
Draft

Conversation

TornaxO7
Copy link
Contributor

@TornaxO7 TornaxO7 commented Dec 13, 2023

Closes #95

It's mostly done. I need to fix some bugs (like opening a huge file, somehow breaks the pager and takes a lot of time) and especially hope that AMythicDev/minus#114 gets merged so we have the follow-mode with minus as well :)
I'll also add more docs if you approve the code (I don't want to remove more lines if you don't like them ;-;).

You can try it out if you want :)

but now I should really go to bed

@bensadeh
Copy link
Owner

Hi again @TornaxO7 and thanks for putting this PR together. I haven't tested the branch out yet, but here are some of my thoughts:

  • Since replacing the default pager is quite a large change to the core functionality (or presentation) of the highlighted output, I suggest putting outputting to minus behind a flag like so: tspin --minus app.log. This will allow us to have both, eventually swapping out one as the default pager and putting the other behind a flag later on.
  • Instead of modifying the current code, I'd like to extend the functionality by adding a minus reader trait implementation (This is the open-closed principle).
  • Same with the writer and presentation traits, I'd like to have them because it decouples them nicely and lets us have the logic for stdin and stdout in a more isolated way.
  • One more thought about the writer trait. Is minus reading everything from memory? Currently, tailspin writes a temporary file with the highlighting applied. I don't know how this is done with minus , but if it is all in-memory, it could be an issue with large files as large log files could be larger than the available memory.
  • I like the EOFSignaler you made as its own type, but let's not introduce it in this PR to keep it as small as possible if that's okay.

Let me know if you're stuck on some parts and we'll see if we can simplify or change some things around.

@TornaxO7
Copy link
Contributor Author

Since replacing the default pager is quite a large change to the core functionality (or presentation) of the highlighted output, I suggest putting outputting to minus behind a flag like so: tspin --minus app.log. This will allow us to have both, eventually swapping out one as the default pager and putting the other behind a flag later on.
Instead of modifying the current code, I'd like to extend the functionality by adding a minus reader trait implementation (This is the open-closed principle).
Same with the writer and presentation traits, I'd like to have them because it decouples them nicely and lets us have the logic for stdin and stdout in a more isolated way.

ooooof, I though that we replace less with minus so we don't have any external dependencies anymore and minus will be fully written in rust xD Welp... bad communication from my side. I'll have to revert my changes then... ._.

One more thought about the writer trait. Is minus reading everything from memory? Currently, tailspin writes a temporary file with the highlighting applied. I don't know how this is done with minus , but if it is all in-memory, it could be an issue with large files as large log files could be larger than the available memory.

good point! If I'm seeing it correctly minus buffers the text, which should be displayed in a String but we can fix this by handling a buffer on our own (or use your file) and set the text manually which minus should display.

@TornaxO7
Copy link
Contributor Author

hm... so would you like to have minus as the default pager?

@bensadeh
Copy link
Owner

hm... so would you like to have minus as the default pager?

I think the discussion of whether to have minus as the default pager can come later down the line.

For the first version, I'd like to be able to use it with the --minus flag for example. This way, we can release a new version with this feature and let people test and try out. I can also add an option to set an environment variable to have this as the default for those who want.

ooooof, I though that we replace less with minus so we don't have any external dependencies anymore and minus will be fully written in rust xD Welp... bad communication from my side. I'll have to revert my changes then... ._.

No problem, my friend.

Like I mentioned earlier, I have tried to decouple the highlighting logic from the presentation logic. The idea behind this is that we should be able to easily swap out these parts since they don't have anything to do with each other.

We already have a less presenter, so I suggest we start with creating a minus presenter that will be used when we rung tailspin with the --minus flag.

good point! If I'm seeing it correctly minus buffers the text, which should be displayed in a String but we can fix this by handling a buffer on our own (or use your file) and set the text manually which minus should display.

I suspect that having minus read from our temporary file will solve any memory issues, but I might be on thin ice here. In any case we need to take into account large files.

So to sum up, a way forward could be creating a minus presenter which implements the Present trait and still reading from the temporary file the same way less is doing. It will be used with the --minus flag.

Lastly, it is important that the minus is able to pick up newline entries somehow. This is already handled with the temp file, as we continuously watch the original file and write to the temp file if new entries come in.

@bensadeh bensadeh mentioned this pull request Jan 5, 2024
@bensadeh
Copy link
Owner

bensadeh commented Jan 5, 2024

I just noticed that minus does not allow to read directly from file like less does, so my approach above would not work in the current version.

I still think it is worth looking more into. I'd like to take another look at integrating minus when I have time. I've tried to make tailspin be agnostic when it comes to how things are presented (to pager vs. stdout etc.). How we display the output can be configurable. It would be interesting to get user feedback on how the minus pager works.

So to sum up, I'll have a look at this again as well, and if there aren't any big blockers (performance, memory etc.) we can look into integrating tighter with tailspin.

Open to thoughts and your ideas as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using minus instead of less?
2 participants