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

Add "Log messages to the console" recipe #69

Closed
wants to merge 2 commits into from

Conversation

rap2hpoutre
Copy link
Contributor

See https://github.com/brson/rust-cookbook/issues/61

I choose to replace "Log a debug message to the console" and "Log an error message to the console" by just "Log messages to the console" (which covers info, warn and error), because recipe is (imo) almost the same for the various message levels. Maybe I'm wrong.

I'm still learning and need mentoring, maybe the quality of my PR is a bit poor. Anyway, thanks to @budziq for his precious help.

I could add a text description around this example later, but today I'm not really inspired (and to be fair I'm not fluent at all in english, so I'm a bit scared).

Copy link
Collaborator

@budziq budziq left a comment

Choose a reason for hiding this comment

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

src/app.md Outdated
@@ -3,8 +3,7 @@
| Recipe | Crates | Categories |
|--------|--------|------------|
| [Parse command line arguments][ex-clap-basic] | [![clap-badge]][clap] | [![cat-command-line-badge]][cat-command-line] |
| [Log a debug message to the console][ex-log-debug] | [![log-badge]][log] [![env_logger-badge]][env_logger] | [![cat-command-line-badge]][cat-command-line] |
| [Log an error message to the console][ex-log-error] | [![log-badge]][log] [![env_logger-badge]][env_logger] | [![cat-command-line-badge]][cat-command-line] |
| [Log messages to the console][ex-log-messages] | [![log-badge]][log] [![env_logger-badge]][env_logger] | [![cat-command-line-badge]][cat-command-line] |
Copy link
Collaborator

Choose a reason for hiding this comment

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

badges do not reflect the used crates please see the env_logger-badge

also [![cat-command-line-badge]][cat-command-line] should be changed to [![cat-debugging-badge]][cat-debugging]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks again for your help. So you mean replacing:

| [Log messages to the console][ex-log-messages] | [![log-badge]][log] [![env_logger-badge]][env_logger] | [![cat-command-line-badge]][cat-command-line] |

By

| [Log messages to the console][ex-log-messages] | [![log-badge]][log] | [![cat-debugging-badge]][cat-debugging] |

Ok I could do that!

src/app.md Outdated
## Log a debug message to the console
[ex-log-messages]: #ex-log-messages
<a name="ex-log-messages"></a>
## Log messages to the console

[![log-badge]][log] [![env_logger-badge]][env_logger] [![cat-command-line-badge]][cat-command-line]
Copy link
Collaborator

Choose a reason for hiding this comment

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

spurious env_logger-badge

@rap2hpoutre
Copy link
Contributor Author

rap2hpoutre commented May 11, 2017

@budziq

as mentioned in #61 (comment) i would suggest making it a more advanced separate example. Something along the lines of "Creating a custom logger". As the trivial consol logging would be implemented in 5 line env_logger snippet

Then in #61:

I would argue that this example is more involved (and rich with important usecase) than minimal (~5 lines) one that would use env_logger. But one of the maintainers would have to decide ;)

I think I totally misunderstood what you wrote in https://github.com/brson/rust-cookbook/issues/61 AND I think I still don't understand! I thought you said "Your example is OK because it has some details". But now, I'm lost.

Thinking about it, I'm not sure anymore I have a sufficient level to do such a PR (english level+rust level+brain level). Honestly, what do you think? Can I give up? (cause maybe it's a waste of time, somebody else would have done better work) If not, will you have the patience to continue to help me?

Thanks again for your help!

@budziq
Copy link
Collaborator

budziq commented May 11, 2017

@rap2hpoutre You should definitely not give up 🥇 as you PR is almost perfect, and please note that this repository is an outreach program for people to learn and be a first step in contribution.

Honestly my english not top notch, so we might have misunderstood, so let me rephrase:

  • Basic logging can be implemented in a more standard way by utilizing env_logger with the smallest number of lines. The first super basic log examples should be similar to this one
  • Your example is a separate usecase that shows users how to create a basic custom logger that might write to file/ring bells or etc. And IMHO your example (after some polishing) could be included in the cookbook.

@rap2hpoutre
Copy link
Contributor Author

rap2hpoutre commented May 11, 2017

(edited)

@budziq OK so I could:

  1. Re-add the links to (void) "Log a debug message to the console" and "Log an error message to the console" and re-add their "placeholders" (because these recipes are another use cases, and mine is a new one)
  2. Add one new link to "Log messages in a custom format"
  3. Rename my example to "Log messages in a custom format"
  4. Add a small description like: "Set a basic custom logger ConsoleLogger and log various messages to it" (not sure about the word "to")
  5. Tweak something trivial about this example to make it custom: println!("Rust says: {} - {}", record.level(), record.args());
  6. Update intro.md after everything is OK

Right?

@dtolnay
Copy link
Member

dtolnay commented May 11, 2017

I agree with @budziq, this example is really valuable but not as the very first log example that we show. I think env_logger would be a better fit for "I just want the log messages to show up." Then we can have this example to show how to define a custom logger.

@rap2hpoutre please rename this example to something like "Log messages in a custom format" and tweak something trivial about it to make it custom, like:

println!("Rust says: {} - {}", record.level(), record.args());

Then please add an example above this one showing a minimal very basic use of env_logger.

@rap2hpoutre
Copy link
Contributor Author

@dtolnay Ok thanks. I updated my todo-list above your comment. Ok with this?

@dtolnay
Copy link
Member

dtolnay commented May 11, 2017

Yes that covers everything. Thank you!

@rap2hpoutre
Copy link
Contributor Author

Ok I will try to do it tomorrow!

@rap2hpoutre
Copy link
Contributor Author

I close this PR because:

  • It's in conflict, and it has no value to resolve it cause I made mistakes in my commits
  • My commits were dirty ("oops")
  • I deleted lines that I did not have to delete
  • The title, description, etc. are not anymore what it's about.

New clean (I hope) PR here: #74 (with the right title, description, no conflict, no random commit messages, etc.) Thanks to @budziq & @dtolnay for their help!!

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.

3 participants