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

Implement MORE command and fix for LS #1556

Closed
wants to merge 4 commits into from
Closed

Implement MORE command and fix for LS #1556

wants to merge 4 commits into from

Conversation

Wengier
Copy link
Sponsor Collaborator

@Wengier Wengier commented Feb 3, 2022

This adds MORE command so that commands like MORE FILE.TXT, MORE < FILE.TXT and DIR | MORE will work. Also added fix for LS command so that one file is shown on one line when piping to MORE command (or when /1 option is used) as well as help message for MORE command.

@shermp
Copy link
Collaborator

shermp commented Feb 3, 2022

Thanks for implementing this @Wengier

I'm not going to do an in depth review of this yet, because I feel that the void MORE::Run() function requires further improvements.

It looks like to me that the file reading, and pipe/redirection reading could be split out into their own functions, unless you can find a way to add more commonality between the two code paths.

I realize that goto can be a necessary evil in C code sometimes (heck, I've used it myself!), but in this case it looks like it's basically used for looping if I'm reading the code right, and that's what loops are for. If you're concerned about nesting, that's where the previous suggestion of splitting things into more functions helps.

@MasterO2
Copy link
Contributor

MasterO2 commented Feb 3, 2022

@Wengier Will there be a similar PR for the LESS command as well? As you know, LESS allows scrolling up and down for directories and text files.

@Wengier
Copy link
Sponsor Collaborator Author

Wengier commented Feb 3, 2022

@MasterO2 LESS is never a standard DOS command (unlike MORE). So I wonder if it should be added as a builtin command (if it is added, many more non-standard commands may be added too).

@Wengier
Copy link
Sponsor Collaborator Author

Wengier commented Feb 3, 2022

@shermp Thanks for the feedback. I will clean up the command further.

@kcgen
Copy link
Member

kcgen commented Feb 3, 2022

If adding LESS entails just a couple more lines of code, then sure. But I believe LESS is a lot more code: so adding this would fall outside our Scope.

There might be a nearly endless treadmill to re-implement any other GNU shell commands as well, right after LESS. Even if an army of volunteer developers materialized to perform this work - who is going to review and maintain those applications? I certainly won't be pouring my own hours into reviewing that work (even if it existed) - it would grind my own progress to a halt and harm Staging's pursuit of its in-scope tasks.

@MasterO2 are there free options you can add that satisfy your need for something like LESS?

@MasterO2
Copy link
Contributor

MasterO2 commented Feb 3, 2022

If adding LESS entails just a couple more lines of code, then sure. But I believe LESS is a lot more code: so adding this would fall outside our Scope.

There might be a nearly endless treadmill to re-implement any other GNU shell commands as well, right after LESS. Even if an army of volunteer developers materialized to perform this work - who is going to review and maintain those applications? I certainly won't be pouring my own hours into reviewing that work (even if it existed) - it would grind my own progress to a halt and harm Staging's pursuit of its in-scope tasks.

@MasterO2 are there free options you can add that satisfy your need for something like LESS?

@kcgen I already use the LESS command from FreeDOS. I only brought it up because I felt that LESS's being natively part of Staging would be immensely helpful for viewing text files, as the MORE command does not allow scrolling backwards.

@Wengier Wengier marked this pull request as draft February 3, 2022 19:37
@Wengier
Copy link
Sponsor Collaborator Author

Wengier commented Feb 3, 2022

@MasterO2 LESS command is never a standard DOS command, and I think few people would use it in DOS. There are also better alternatives for viewing text in DOS.

@Grounded0
Copy link
Collaborator

Grounded0 commented Feb 3, 2022

If someone wants to use LESS they can install it themselves from FreeDOS repo. Its not part of FreeDOS Base package group but part of Unix-like package group:

https://www.ibiblio.org/pub/micro/pc-stuff/freedos/files/distributions/1.2/repos/pkg-html/group-base.html

https://www.ibiblio.org/pub/micro/pc-stuff/freedos/files/distributions/1.2/repos/pkg-html/group-unix.html

DOS is DOS and Unix is Unix.

@kcgen kcgen requested a review from shermp February 14, 2022 19:47
@kcgen kcgen added the enhancement New feature or enhancement of existing features label Feb 14, 2022
@kcgen kcgen added this to In progress in 0.79 release via automation Feb 14, 2022
@kcgen
Copy link
Member

kcgen commented Feb 14, 2022

@shermp Thanks for the feedback. I will clean up the command further.

Just wanted to say I agree with the approach guys, and look forward to the next iteration!

@MasterO2
Copy link
Contributor

Any progress on this one, @Wengier?

@Wengier
Copy link
Sponsor Collaborator Author

Wengier commented Mar 15, 2022

@MasterO2 I also have other PRs to work on; will get back to this.

Copy link
Member

@kcgen kcgen left a comment

Choose a reason for hiding this comment

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

The state of this PR is pending addressing the comments mentioned here:

#1556 (comment)

(No rush; just talking another pass through all the open PRs.)

@kcgen
Copy link
Member

kcgen commented Jun 21, 2022

As this PR has been idle for some time, I'm going to park it off the main page, however the PR and the work is very welcome, when the time is right to keep progressing it. We can open and carry on then. Thank you @Wengier !

@kcgen kcgen closed this Jun 21, 2022
0.79 release automation moved this from In progress to Done Jun 21, 2022
@kcgen kcgen deleted the ww/more branch January 19, 2023 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or enhancement of existing features
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants