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

refactor: rename "logs" to "logLines" #1324

Closed
wants to merge 1 commit into from
Closed

Conversation

bajtos
Copy link
Member

@bajtos bajtos commented Feb 9, 2024

Make it clear in code storing logs that chunks must be split at newlines.

My intention is to prevent regressions like the one fixed by #1319.

Make it clear in code storing logs that chunks must be split at
newlines.

Signed-off-by: Miroslav Bajtoš <oss@bajtos.net>
@bajtos bajtos requested a review from juliangruber February 9, 2024 14:03
Copy link
Member

@juliangruber juliangruber left a 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 this is the way to go. While I agree the needs a refactor to prevent regressions, I think the fault lies inside the abstraction. To me, the library is still for logs (it's an implementation detail whether it's storing individual lines or not). Then, I see too ways to make it safer:

a) Change the method names to Logs#pushLine() and Logs#getLastLines()
b) Inside Logs#push(), split strings on newlines, so that whatever can be pushed to it

What do you think?

@bajtos
Copy link
Member Author

bajtos commented Feb 12, 2024

I don't think this is the way to go. While I agree the needs a refactor to prevent regressions, I think the fault lies inside the abstraction. To me, the library is still for logs (it's an implementation detail whether it's storing individual lines or not).

👍🏻

Then, I see too ways to make it safer:

a) Change the method names to Logs#pushLine() and Logs#getLastLines()

I did consider this option, too, and I am happy to adopt it.

b) Inside Logs#push(), split strings on newlines, so that whatever can be pushed to it

This goes beyond a simple cleanup, it will require a bit more work & testing, which I don't have appetite for right now.

@juliangruber
Copy link
Member

I did consider this option, too, and I am happy to adopt it.

Go for it! 🙏

@bajtos
Copy link
Member Author

bajtos commented Feb 13, 2024

Closing in favour of #1325

@bajtos bajtos closed this Feb 13, 2024
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.

2 participants