serenity icon indicating copy to clipboard operation
serenity copied to clipboard

Published 20 hours ago • verified publisher icon SerenityOS

cat: Add -b to number non-blank lines

Open ebanner opened this issue 1 year ago • 5 comments

This PR adds a -b option to the cat command to number non-blank lines:

image

This is one of two implementations I came up with. It diverges from the existing approach of determining when to print a line number and opts for a more explicit state machine-based approach.

The other PR more closely resembles and extends the existing approach https://github.com/SerenityOS/serenity/pull/24917

I personally think this approach easier to understand than trying to extend the existing approach.

ebanner avatar Aug 09 '24 14:08 ebanner

Hello!

One or more of the commit messages in this PR do not match the SerenityOS code submission policy, please check the lint_commits CI job for more details on which commits were flagged and why. Please do not close this PR and open another, instead modify your commit message(s) with git commit --amend and force push those changes to update this PR.

BuggieBot avatar Aug 09 '24 14:08 BuggieBot

@PerrinJS do you have thoughts on this approach versus https://github.com/SerenityOS/serenity/pull/24917?

ebanner avatar Aug 12 '24 13:08 ebanner

@nico would you be willing to take a look? I see you helped review the cat -n PR https://github.com/SerenityOS/serenity/pull/24382

ebanner avatar Aug 19 '24 13:08 ebanner

I like the approach, and it looks good. However, adding the -n was my first commit to this project, so I'm not fully sure about working as a reviewer when I don't know all the rules of the project. Especially when it comes to how adding test units is suposed to be done.

PerrinJS avatar Aug 20 '24 13:08 PerrinJS

@nico would you be willing to take a look? I see you helped review the cat -n PR #24382

CI is currently red. Can you get that sorted out before asking for reviews?

nico avatar Aug 21 '24 02:08 nico

Fixed the CI check. PR is ready for review

ebanner avatar Aug 29 '24 20:08 ebanner

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions!

stale[bot] avatar Sep 28 '24 05:09 stale[bot]

This pull request has been closed because it has not had recent activity. Feel free to re-open if you wish to still contribute these changes. Thank you for your contributions!

stale[bot] avatar Oct 05 '24 18:10 stale[bot]