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

lp: add page #321

Closed
wants to merge 2 commits into from
Closed

lp: add page #321

wants to merge 2 commits into from

Conversation

Qtrain
Copy link
Contributor

@Qtrain Qtrain commented Nov 8, 2015

fixed and deleted line in shell script that was giving me problems. Let me know if you need changes. thx.

@waldyrious waldyrious mentioned this pull request Nov 8, 2015

- Print the output of a command.

`echo "test" | lp -d {{printer_name}}`
Copy link
Member

Choose a reason for hiding this comment

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

-d is demonstrated below, so I'd suggest leaving this example as the simpler echo "test" | lp (which also applies the "progressive complexity" principle). If you change this, make sure to add "to the default printer" to the command description.

@Qtrain
Copy link
Contributor Author

Qtrain commented Nov 10, 2015

Hi. I implemented all of the changes. I got rid of the lpstat -d command as it appeared to be redundant. Let me know what you think. Thanks.


`lp -d {{printer_name}} {{path/to/filename}}`

- Print N copies of file to default printer (replace N with desired number of copies).
Copy link
Member

Choose a reason for hiding this comment

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

"of file" --> "of a file"
"to default" --> "to the default"

@waldyrious
Copy link
Member

@Qtrain how about the suggestion not to use lpstat as a separate example but instead mention the commands in the descriptions for the corresponding examples? And I am still not sure about the purpose of the edit to lint-changed.sh.

@Qtrain
Copy link
Contributor Author

Qtrain commented Nov 10, 2015

@waldyrious I'm not sure what you mean. I was under the impression that the descriptions shouldn't have commands in them. So, I'm not really sure how you want me to change it. Would you be able to provide an example?

Additionally, I added that line at the suggestion of @lord63 please see above conversation.

I apologize for the sprawling commit here to btw. This is my first time attempting to commit to an open source repo so any "hand-holding" is greatly appreciated. Thx!

@waldyrious
Copy link
Member

@waldyrious I'm not sure what you mean. I was under the impression that the descriptions shouldn't have commands in them. So, I'm not really sure how you want me to change it. Would you be able to provide an example?

Sure, I meant something like this:

- Print the output of a command to the default printer (use `lpstat -d` to see what the default printer is)

`echo "test" | lp`

- Print a file to a named printer (use `lpstat -p | awk '{print $2}'` to list available printers)

`lp -d {{printer_name}} {{path/to/filename}}`

@rprieto
Copy link
Contributor

rprieto commented Nov 11, 2015

I'm actually not in favour of having other commands in the description (especially complex expressions with pipes). I'd rather keep it simple and say (see lpstat command). And then people can tldr lpstat if they don't know about it / need a refresher.

@waldyrious
Copy link
Member

I honestly think we need to balance between general principles (not having commands in descriptions, which I agree) and what's more useful for the user here (straight up giving them the answer they need). "see lpstat command" forces them to an extra step and is not much better than the RTFM problem which tldr aims to alleviate. In this specific case I believe the commands are justified, especially since they're so short and simple.

@Qtrain
Copy link
Contributor Author

Qtrain commented Nov 21, 2015

hello, I added another commit. Please let me know what you think. Thanks.

@waldyrious
Copy link
Member

@Qtrain you mean you started yet another PR, right? I'm starting to get a little lost since now there are loose threads all over the place. Why don't you keep the conversation in a single place? :(

Btw @rprieto what are your thoughts regarding my last comment above?

@Qtrain
Copy link
Contributor Author

Qtrain commented Nov 21, 2015

@waldyrious

Sorry for all of the novice mistakes. I deleted, re-forked and re-cloned the repository in an effort to get around the error with this line

-[[ -z "$MD_FILES" ]] || GEM_PATH=.gem .gem/bin/mdl "$MD_FILES"

However I still received the error and the only way I could get it to work was by replacing the above line with this .

MD_FILES=git diff --cached --name-only | tr " " "\n" | grep '^.*\.md$'
+# Execute Markdown lint if any markdown files have been changed and added to git
+[[ -z "$MD_FILES" ]] || mdl "$MD_FILES"
+exit 0

So, as you can see, I am obviously making mistakes, but, I’m just not sure how to do it correctly, and I really don’t know how to research the commit process given that I don’t even understand the above code. Again my apologies. However, I did add an lpstat file to the commit :)

On Nov 21, 2015, at 1:51 PM, Waldir Pimenta notifications@github.com wrote:

@Qtrain https://github.com/Qtrain you mean you started yet another PR, right? I'm starting to get a little lost since now there are loose threads all over the place. Why don't you keep the conversation in a single place? :(

Btw @rprieto https://github.com/rprieto what are your thoughts regarding my last comment above?


Reply to this email directly or view it on GitHub #321 (comment).

@rprieto
Copy link
Contributor

rprieto commented Nov 22, 2015

I agree with being pragmatic, but all in all still stand by my previous comment:

  • the examples should be for the current command. I don't expect an example in the lp page that actually doesn't use lp - in this case for example no having lpstat as a standalone example. These belong in the lpstat page.
  • I'd rather not have other commands in the description section. I'm happy with (see lpstat) but not really with call lpstat -p | awk '{print $2}'. It takes 5sec for the user to call tldr lpstat. Otherwise we encourage duplication of examples from their own page to other potentially-related page, which is a slippery slope.

I think "giving the answer they need" is tricky because we don't know what people need. Some people might just want a refresher on how lp works (90% of my personal use at least). If some commands are so tightly related that 2 of the examples need to mention how to call the other command, people should probably read both tldr entries to understand what they do anyway. My 2 cents :)

@rprieto
Copy link
Contributor

rprieto commented Nov 22, 2015

Otherwise, keen to merge this page as soon as we can. @Qtrain's put a lot of work into it and it's great to get new commands 👍 We can always adjust a few things later.

@waldyrious
Copy link
Member

@rprieto fair enough, I can agree with that. 👍 to merging (note #324 though).

@Qtrain thanks for the explanation. Indeed some of these errors are tricky. Kudos for the good work and patience :)

@Qtrain
Copy link
Contributor Author

Qtrain commented Nov 22, 2015

@rprieto @waldyrious Thanks for the feedback and bearing with me on this mess (cringe). Please let me know what next steps need to be taken. I don't want to end up accidentally submitting yet another pull request. Thanks.

@Qtrain
Copy link
Contributor Author

Qtrain commented Dec 1, 2015

@rprieto @waldyrious Hi guys, I would still like to get this committed if possible. I realize that I am behind the branch now, so, I was hoping one of you might be able to provide some direction. Please let me know. Thanks.

@igorshubovych
Copy link
Collaborator

@Qtrain,

Thank you very much for a valuable contribution!

I had to make new commit instead of yours because the branches diverged too much :)

I used your text, but changed the structure:

  • There are separate commands: lp and lpstat.
  • I removed awk as @rprieto adviced. awk is not really related to lpstat. In the end you can fetch this info using cut, other interpreters or just your eyes.
  • But I did not removed echo. @rprieto, do you have any ideas how to replace it?
  • Example lpstat {filename} is fixed to lp {filename}. The 1st variant did not work for me (and it was wrong according to manual).
  • The rest is taken from your commits.

If you want you are welcome to submit further pull requests to improve these pages.

Keep the good work!

@Qtrain
Copy link
Contributor Author

Qtrain commented Dec 1, 2015

@igorshubovych No worries! Thanks very much for taking the reigns, I really appreciate your help! Cheers

igorshubovych added a commit to igorshubovych/tldr that referenced this pull request Dec 3, 2015
@waldyrious waldyrious mentioned this pull request Jan 1, 2016
@waldyrious waldyrious added new command Issues requesting creation of a new page. decision A (possibly breaking) decision regarding tldr-pages content, structure, infrastructure, etc. architecture Organization of the pages per language, platform, etc. labels Sep 16, 2016
@waldyrious waldyrious changed the title print command print: add page Sep 28, 2016
@waldyrious waldyrious changed the title print: add page lp: add page Sep 28, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
architecture Organization of the pages per language, platform, etc. decision A (possibly breaking) decision regarding tldr-pages content, structure, infrastructure, etc. new command Issues requesting creation of a new page.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants