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

rtcwake: add page #2098

Merged
merged 6 commits into from
May 10, 2018
Merged

rtcwake: add page #2098

merged 6 commits into from
May 10, 2018

Conversation

FraYoshi
Copy link
Contributor

@FraYoshi FraYoshi commented May 6, 2018

Added rtcwake Linux page and examples

  • 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.

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.

Hey, thanks for the new page! I've left a comment or two below for you to review.

You don't need to open a fresh pull request - simply appending commits to this one is fine (we've got a special button that squashes them all for us, so you don't need to worry about that) - you can even use GitHub's web interface!

Looks like you've had some issues with the linter. I'm not sure on that "command descriptions should begin with a capital letter" problem, but normally you should eb able to push a new commit and it will re-check for you, rather than opening a new pull request.

The @tldr-bot account actually just reports the result from the continuous-integration testing (which I think runs through Travis CI IIRC).


`rtcwake -m show -v`

- Show if an allarm is set:
Copy link
Member

Choose a reason for hiding this comment

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

Typo here? allarm -> alarm

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, typo.. gonna correct thanks =)


- Suspend to ram and wakeup after 10 seconds:

`# rtcwake -m mem -s {{10}}`
Copy link
Member

Choose a reason for hiding this comment

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

Is the hash needed here? If it's denoting a superuser command, perhaps you could use sudo instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's needed to be superuser, sincerely, I was unsure about using the hash or the sudo.. Gonna switch to the last one =)


`# rtcwake -m mem -s {{10}}`

- Suspend to disk _(higher power saving)_ and wakeup 10 minutes later:
Copy link
Member

Choose a reason for hiding this comment

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

Not sure on italics and whether it renders correctly in the terminal. @agnivade, could you confirm?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think it usually does, at least not with a few of the clients. 🤔

Copy link
Member

Choose a reason for hiding this comment

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

It works fine on the node client. But usually we avoid markdown in the description, and this seems to be an incorrect usage of italicizing a text anyways. We should remove the underscores.


`# rtcwake -m disk --date +{{15}}m`

- Suspend to disk and wakeup 1 hour later so play some music with mpv:
Copy link
Member

Choose a reason for hiding this comment

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

Maybe ....so play some music with mpv would read better as ....and play some music with mpv?

Copy link
Contributor Author

@FraYoshi FraYoshi May 6, 2018

Choose a reason for hiding this comment

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

I was also unsure about the use of mpv.. It's a cool combination but maybe "and run a command" could be better because not every system has mpv installed.. i.e. they could have vlc

@owenvoke owenvoke added the new command Issues requesting creation of a new page. label May 6, 2018
alarm typo. hashes to sudo. italics to plain text. "so play" to "and play".
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! Thanks, @FraYoshi :D

Copy link
Member

@agnivade agnivade left a comment

Choose a reason for hiding this comment

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

Lots of comments. This page introduces lot of concepts in an out-of-order fashion. I just wanted to clarify things a bit.

Thanks for the page !

# rtcwake

> Enter a system sleep state until specified wakeup time relative to your bios clock.
> Commands need to be run as superuser since shutdowns are involved.
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 it's better to have all the commands have the sudo prefix. Currently, only some of them have it which is incorrect. Then we can remove this line which is redundant information.

> Enter a system sleep state until specified wakeup time relative to your bios clock.
> Commands need to be run as superuser since shutdowns are involved.

- Check your hardware clock info and exit:
Copy link
Member

Choose a reason for hiding this comment

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

This and the example after this are the same. I think this one is not necessary to be in a tldr page.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So removing It but keeping the -v flag since avoiding implies a CTRL-C command after.


`rtcwake -m show -v`

- Show if an alarm is set:
Copy link
Member

Choose a reason for hiding this comment

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

This can mean that show only if an alarm is set. Perhaps - "Show whether an alarm is set or not:" is better.


`rtcwake -m show`

- Disable a previous set alarm:
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be after the examples where an alarm is set. We always mention examples in the order of usage. So you cannot disable an alarm if it is not set in the first place.

Also - "Disable a previously set alarm:"


`sudo rtcwake -m mem -s {{10}}`

- Suspend to disk (higher power saving) and wakeup 10 minutes later:
Copy link
Member

Choose a reason for hiding this comment

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

The description mentions 10 minutes, but example has 15 minutes. Please choose one.

Also, we should be consistent in the language. The description before has "after 10 seconds", but this one has "10 minutes later". We should choose one and be consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I thought that adding more examples was better for a higher understanding of how the thing works.. so s for seconds, m for minutes and so on.. Or maybe Is better a longer description?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or maybe a standard time like 1h and the thing written in 3 different ways in 3 different examples like: 3600s, 60m and 1h!

Copy link
Member

Choose a reason for hiding this comment

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

We already have 15m and 1h, so I think it is clear enough that different units can be used.


`sudo rtcwake -m disk --date +{{15}}m`

- Suspend to disk and wakeup 1 hour later and play some music with mpv:
Copy link
Member

Choose a reason for hiding this comment

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

Why is this example necessary ? I don't see it adding anything new except using hour as a unit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a useful usage example imo... for example, I use the command mainly as a wake-up alarm.

Copy link
Member

Choose a reason for hiding this comment

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

This is showing composition of one command with another command. You are combining the wakeup action with playing music after it to make it behave like an alarm. Strictly speaking, this behavior is outside the scope of a tldr page for rtcwake, which should just be focused on explaining the command usage. A user can easily construct an example like this if he/she understands the command well enough (which should be our primary aim).


`sudo rtcwake -m freeze --date {{YYYYMMDDhhmm}}`

- Run a test (CTRL-C to abort) in which the computer is waked at a given time:
Copy link
Member

Choose a reason for hiding this comment

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

What does the "test" do ? Is this sort of a dry run ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, a sort of dry run. So maybe better adding It at the top?

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 fine. But perhaps we can modify the description to - Perform a dry run to wakup the computer at a given time. (Press Ctrl + C to abort):

Added "sudo" to all the commands so removed the second line. Removed a redundant example. Modified a description. Changed orders. Still to define some esamples involving a different time in s, m, h....
long form of the time unit: m → min.
removed a composited example.
modified last description about the test.
@FraYoshi
Copy link
Contributor Author

Now It should be more clear.
I take a moment to thank you for enduring my mistakes ^^"

now there's an example involving -s (seconds) and one involving --date (time_units)
do you think It's fine keeping both?

@agnivade
Copy link
Member

Yes it's fine. Thanks.

@agnivade
Copy link
Member

@sbrl / @pxgamer - PTAL. Merge if you think it's fine.

Copy link
Member

@owenvoke owenvoke left a comment

Choose a reason for hiding this comment

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

Looks good to me. 👍

@owenvoke owenvoke merged commit 52a8e63 into tldr-pages:master May 10, 2018
lamar-frankie pushed a commit to lamar-frankie/tldr that referenced this pull request Jun 4, 2018
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