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 with a custom logger" recipe #74

Merged
merged 3 commits into from
May 12, 2017

Conversation

rap2hpoutre
Copy link
Contributor

@rap2hpoutre rap2hpoutre commented May 12, 2017

This recipe shows how to create a custom logger and log various messages into it. Based on #69 and is a part of #61

Side note:

Too many errors and mistakes were made in https://github.com/brson/rust-cookbook/pull/69 (and it's now in conflit with commit like "oops", so its quality is poor). Now with the help of @dtolnay and @budziq I re-created a clean (?) PR based on this todo-list https://github.com/brson/rust-cookbook/pull/69#issuecomment-300814527

@budziq
Copy link
Collaborator

budziq commented May 12, 2017

@rap2hpoutre Hi there was no deed to close the original PR as you could have just git push --force pulled_branch to your repository with new changes and the PR would be completely updated/rewritten :)

Anyhow. Your pull looks very good 😃 . Minor suggestions:

@rap2hpoutre
Copy link
Contributor Author

rap2hpoutre commented May 12, 2017

@budzik Thanks! Once again I would like your recommandations. What about this textual description:

Set a basic custom logger ConsoleLogger that logs various messages to stdout. Loggers implement the log::Log trait and are installed by calling the log::set_logger function. This ConsoleLogger writes to stdout.

Source: https://doc.rust-lang.org/log/log/index.html#logger-implementations

For the other suggestion, I think you are right but I think we do not need "impl", just "Log messages with a custom logger"

@budziq
Copy link
Collaborator

budziq commented May 12, 2017

I would suggest to make the text a little more concise and in passive voice.

Custom logger ConsoleLogger is implemented with log::Log trait and installed via log::set_logger. Messages are logged to stdout.

As to your suggestion "Log messages with a custom logger" looks awesome to me.

For future reference, I would just push updated PR and ask for review instead of posting suggested text inline. It will be easier and less noisy on the thread :)

@rap2hpoutre rap2hpoutre changed the title Add "Log messages in a custom format" recipe Add "Log messages with a custom logger" recipe May 12, 2017
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.

Almost there :)

  • forgotten to update master table in intro.md
  • use ex-log-custom

Please use try to use git commit --amend and then git push --force to update history without new commit.

src/app.md Outdated
@@ -5,7 +5,7 @@
| [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-debugging-badge]][cat-debugging] |
| [Log an error message to the console][ex-log-error] | [![log-badge]][log] [![env_logger-badge]][env_logger] | [![cat-debugging-badge]][cat-debugging] |
| [Log messages in a custom format][ex-log-custom] | [![log-badge]][log] | [![cat-debugging-badge]][cat-debugging] |
Copy link
Collaborator

Choose a reason for hiding this comment

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

ex-log-custom was ok no need to change it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In fact I actually had to change ex-log-custom because it was already in use below and I used ex-log-logger everywhere else. I commited (with --amend).

Copy link
Collaborator

Choose a reason for hiding this comment

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

then use ex-log-custom-logger ;) ex-log-logger is not really informative

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I just changed from ex-log-custom-logger to ex-log-logger

@budziq
Copy link
Collaborator

budziq commented May 12, 2017

Looks good to me 👌. Great work! I assure you that it was painful only the first time :)

@rap2hpoutre Do you feel upto creating the the minimal env_logger based examples?

@rap2hpoutre
Copy link
Contributor Author

rap2hpoutre commented May 12, 2017

Great work! I assure you that it was painful only the first time :)

Thank YOU for all the time you offered me. I learned A LOT! And I am slow so thanks for your patience.

Do you feel upto creating the the minimal env_logger based examples?

Not today I think, maybe after the weekend. I think I will do this in a new PR because I can't say "yes I will do this in X hours/days", I have no idea right now. But I will continue to contribute and hope I will become a better contributor.

@dtolnay dtolnay merged commit e8581e5 into rust-lang-nursery:master May 12, 2017
@dtolnay
Copy link
Member

dtolnay commented May 12, 2017

Nicely done!

@dtolnay dtolnay mentioned this pull request May 12, 2017
7 tasks
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.

None yet

3 participants