Skip to content

Conversation

@dtruong33
Copy link

@dtruong33 dtruong33 commented Sep 24, 2024

Implemented man command and framework for adding man pages. Input a command or function's name into the command to bring up its man page, automatically finding file if it exists and automatically applying the file extension. Additionally can use the -f flag to find a specific word.

Changed Files:

  • user/man.c: Man command itself
  • Makefile: Recognizes man.md
  • man.md: Example man page
  • mkfs.c: Allows docs directory to be made

Flags:

  • (-f): find specific word in man page

Limitations:

  • No scrolling, prints the entire man page at once
  • Find only prints out the lines in which the found word is contained

@rick2244
Copy link

I would like to code review this

@chansrinivas
Copy link

I can code review this

@dtruong33
Copy link
Author

Updated man command with support for using docs as a directory to hold man pages as well as various bug fixes and updating

@rick2244
Copy link

I don't see man.c in user

@jlazaro2
Copy link

i can also code review this

@chansrinivas
Copy link

chansrinivas commented Oct 1, 2024

I think your project is great and really useful! The features outlined in the issue are completed. One of the limitations is the scrolling feature which was earlier mentioned in the issue. The man page is printed all at once but it looks good this way too! For flags, I would say it's a good idea to add a "usage" message if the -F flag isn't passed in correctly as args to the program. Everything else looks good, javadoc comments are included as well! Another suggestion, it would be a good idea to have a couple more files in the docs folder for other common commands like cat, ls, cd and so on.

High Level Checks:

  • PR fully resolves the task/issue
  • Does the PR have tests?
  • Does the PR follow project code formatting standards?
  • Does the OS still compile and run?
  • Is documentation included?

Code Checks:

  • Does the code achieve its intended purpose?
  • Were all edge cases considered?
    mkfs.c and Makefile are modified to add new docs in the xv6 system. I think having more tests is a good idea. Right now,
    your docs folder only has man.md so only that can be tested.
    Perhaps having sample man pages for commands like LS and CD would be a good idea so that it could be tested without
    needing to modify the mkfs.c/Makefile to add more documents to the file system.
  • Is the code organized and maintainable?
  • Does the code maintain the same level of performance? (no performance regressions)

@jlazaro2
Copy link

jlazaro2 commented Oct 3, 2024

This man command does not only work well but prints out the information smoothly. I like how you added a flag to search up specific words and I do believe that your code is both readable and formatted well. While this pull request does meet all the issue goals, it also adds more with the -f flag.
Here's a couple of suggestions:

  • When running just the command "man" without any arguments, it prints the command page still. Instead, I suggest the terminal printing a statement asking for a specific command
  • Similar to the first suggestion, when I run "man man -f", I would suggest printing a statement asking for a specific word
  • While the -f flag achieves it's purpose, I feel like it would be better to search and receive specific information in the command pages instead. For example, if you run "man man -f AUTHOR" it would print out, "AUTHOR: " including the line in which this information is found.

High Level Checks:

  • PR fully resolves the task/issue
  • Does the PR have tests?
    The command man is tested when "man man" is run on qemu. However, there
    were no specific test files stated. I was also unsure how to test the -f flag at
    first until I looked at the examples in the man.md file, so i can assume the command is
    run like "man -f . Still, it worked once I was able to run
    properly so good job!
  • Does the PR follow project code formatting standards?
  • Does the OS still compile and run?
  • Is documentation included?

Code Checks:

  • Does the code achieve its intended purpose?
  • Were all edge cases considered?
    Not exactly, see suggestions up above!
  • Is the code organized and maintainable?
  • Does the code maintain the same level of performance? (no performance regressions)

@malensek
Copy link
Contributor

This is a good idea (I love documentation). However I think the scope got scaled back on this. Going back to #64, we have:

  • The command itself
  • Man page library for commonly used commands and functions
  • Display and formatting for the pages
  • Functionality for scrolling and searching within the man pages

Scrolling is admittedly quite a bit of work, so I think we can set that aside. Still, I think we're lacking a library or some kind of infrastructure for writing these man pages (even a tool that helps format them correctly before being put in fs.img) and the formatting options aren't there. Say for instance, maybe you could support markdown # headers so those can be automatically capitalized and printed with bold text. Word wrapping could be another option (safe to hard code the width of the terminal to 80 characters and then break up lines automatically so the doc author doesn't have to do it).

The main reason I point this out is right now it's a tough sell to use this over just cat and grep -- as another CR mentioned, maybe searching for a specific key could show its value? Plus, I am surprised there is no man page for ls since it's even used as an example. I do see you put effort into making a better find than just stock grep since we get the line number. On the other hand, we lose the basic regex support.

I ran into this quirk:

$ man -f Daniel
Man error: Unknown Flag

If you want to update this, adding basic formatting for headers, a couple more man pages as examples, and fix the argument parsing would net you +0.25 to your project score.

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.

5 participants