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

systemctl: update page #12748

Merged
merged 1 commit into from May 10, 2024
Merged

systemctl: update page #12748

merged 1 commit into from May 10, 2024

Conversation

vanvuvuong
Copy link
Contributor

  • The page(s) are in the correct platform directory: linux
  • The page(s) have at most 8 examples.
  • The page description(s) have links to documentation or a homepage.
  • The page(s) follow the content guidelines.
  • The PR title conforms to the recommended templates.

@vanvuvuong vanvuvuong requested a review from cyqsimon as a code owner May 7, 2024 03:40
@github-actions github-actions bot added the page edit Changes to an existing page(s). label May 7, 2024
Copy link
Collaborator

@cyqsimon cyqsimon left a comment

Choose a reason for hiding this comment

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

Hi, welcome to the project and thanks for your contribution.

The PR looks pretty good, but I have some reservations regarding the description change.

pages/linux/systemctl.md Outdated Show resolved Hide resolved
@cyqsimon
Copy link
Collaborator

cyqsimon commented May 7, 2024

Also as a sidenote, I think this page could use some cleaning up. The inclusion of commands like systemctl mask/unmask and systemctl daemon-reload is questionable due to their very infrequent use.

In my personal opinion and experience, it would be much better to include an example that shows operating on the user daemon (systemctl --user), and an example that shows how to access logs (journalctl). We can discuss this in a separate thread.

@CLAassistant
Copy link

CLAassistant commented May 8, 2024

CLA assistant check
All committers have signed the CLA.

@vitorhcl
Copy link
Member

vitorhcl commented May 8, 2024

In my personal opinion and experience, it would be much better to include an example that shows operating on the user daemon (systemctl --user), and an example that shows how to access logs (journalctl). We can discuss this in a separate thread.

My experience with systemctl is too little to debate which commands we should put in its page, but I agree we should include the most useful commands in its page, as we try to do in all our pages.

Also as a sidenote, I think this page could use some cleaning up. The inclusion of commands like systemctl mask/unmask and systemctl daemon-reload is questionable due to their very infrequent use.

IMO a good solution is to add separate pages for those less used commands and then put in the description that common text that says that some subcommands have their own page, such as examples.

Even though some of them don't have their own options and/or subcommands, a separate page can be useful for showing the most usegul example even if only flobal options are used.

The example will also benefit both from a nice explanation of the purpose of the subcommand and from a nice description of what the example usage actually does :)

Copy link
Member

@vitorhcl vitorhcl left a comment

Choose a reason for hiding this comment

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

In general, the changes look very good to me :)

I have some minor suggestions.

About the issue of the usefulness of the current examples, I agree we can discuss this on another PR/issue.

As a positive sidenote:

I liked the pattern of listing the possible values in the right order when there is enough space for this because it gives for the translators the opportunity of translating the possible values.

This is particularly useful when the reader doesn't know / isn't good at English.

We could maybe use this pattern in other pages too, any more opinions?

pages/linux/systemctl.md Outdated Show resolved Hide resolved
pages/linux/systemctl.md Show resolved Hide resolved
pages/linux/systemctl.md Outdated Show resolved Hide resolved
@acuteenvy acuteenvy changed the title systemctl: add list-units subcommand example systemctl: update page May 8, 2024
@vanvuvuong vanvuvuong force-pushed the main branch 2 times, most recently from fc9effa to f64c3e1 Compare May 9, 2024 09:21
Copy link
Member

@kbdharun kbdharun left a comment

Choose a reason for hiding this comment

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

LGTM, Welcome to tldr and thanks for your contribution.

pages/linux/systemctl.md Outdated Show resolved Hide resolved
pages/linux/systemctl.md Outdated Show resolved Hide resolved
@vanvuvuong vanvuvuong force-pushed the main branch 2 times, most recently from 71e86aa to aaf9b17 Compare May 10, 2024 07:13
@vanvuvuong vanvuvuong requested a review from Managor May 10, 2024 07:14
@cyqsimon
Copy link
Collaborator

Okay let's merge!

@cyqsimon cyqsimon merged commit 6800e10 into tldr-pages:main May 10, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
page edit Changes to an existing page(s).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants