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

modinfo: add page #1585

Merged
merged 5 commits into from
Nov 15, 2017
Merged

modinfo: add page #1585

merged 5 commits into from
Nov 15, 2017

Conversation

aydwi
Copy link
Contributor

@aydwi aydwi commented Oct 26, 2017


  • The page (if new), does not already exist in the repo.

  • The page (if new), has been added to the correct platform folder:
    common/ if it's common to all platforms, linux/ if it's Linux-specific, and so on.

  • The page has 8 or fewer examples.

  • The PR is appropriately titled:
    <command name>: add page for new pages, or <command name>: <description of changes> for pages being edited

  • The page follows the contributing guidelines

@agnivade agnivade added the new command Issues requesting creation of a new page. label Oct 27, 2017
@@ -0,0 +1,7 @@
# modinfo

> Extract information about a Linux Kernel module.
Copy link
Member

Choose a reason for hiding this comment

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

linux kernel. Lower case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made kernel lowercase. I think Linux should stay as it is.


> Extract information about a Linux Kernel module.

- Print value(s) of the specified field(s) instead of all the fields:
Copy link
Member

Choose a reason for hiding this comment

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

I think we should start with modinfo {{kernel_module}} and explain what it does.

and then go to -F.

Also, instead of /, use |.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the description

@stale
Copy link

stale bot commented Nov 11, 2017

Hi all! This thread has not had any recent activity. Are there any updates? Thanks!

@stale stale bot added the waiting Issues/PRs with Pending response by the author. label Nov 11, 2017
@stale stale bot removed the waiting Issues/PRs with Pending response by the author. label Nov 13, 2017

`modinfo {{kernel_module}}`

- List value(s) of the specified field(s) instead of all the fields:
Copy link
Member

Choose a reason for hiding this comment

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

You mention "attributes" above and "fields" in this example. Please use any one to be consistent.

Also, no need to say "instead of all the fields". The first example clarifies it that not setting any flags shows all attributes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated it further. Please check if it is appropriate now.

Copy link
Member

@sbrl sbrl left a comment

Choose a reason for hiding this comment

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

Looks ok to me, apart from one minor point 😺


- List the specified attribute(s) only:

`modinfo -F {{author|description|license|parm|filename}} {{kernel_module}}`
Copy link
Member

Choose a reason for hiding this comment

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

It's a bit ambiguous here how multiple attributes are delimited. Is it with the vertical bar?

Copy link
Member

Choose a reason for hiding this comment

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

Yes the vertical bar signifies multiple values which can be used. It's what we have been using.

Copy link
Member

Choose a reason for hiding this comment

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

Hrm - I'm talking about the underlying modinfo call here - the description for this command specifies attribute(s) - implying that multiple values can be specified here.

Copy link
Member

Choose a reason for hiding this comment

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

Ah no, I don't think so. ping @aydwi

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, multiple attributes can be specified, separated by a space.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, then in that case change it to - List the specified attribute(s) only. (You can specify multiple attributes separated by space)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After some testing, it looks like it does not accept multiple attributes in a single command. I've fixed the mistake and pushed again.

@tldr-bot
Copy link

The build for this PR has failed with the following error(s):

pages/linux/modinfo.md:9: TLDR005 Example descriptions should end in a colon

Please fix the error(s) and push again.

@agnivade
Copy link
Member

Thanks for verifying.

@agnivade agnivade merged commit ced7343 into tldr-pages:master Nov 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new command Issues requesting creation of a new page.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants