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.

Sign up for GitHub

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

Jump to bottom

Issue/79 | Add logger #84

Merged
merged 6 commits into from
Nov 21, 2022
Merged

Issue/79 | Add logger #84

merged 6 commits into from
Nov 21, 2022

Conversation

0xBEEFCAF3
Copy link
Contributor

@0xBEEFCAF3 0xBEEFCAF3 commented Nov 20, 2022 •
edited
Loading

fix #79

@0xBEEFCAF3 0xBEEFCAF3 marked this pull request as ready for review November 20, 2022 19:57
@0xBEEFCAF3 0xBEEFCAF3 force-pushed the issue/79 branch 2 times, most recently from 3d2cdd6 to daeb0da Compare November 20, 2022 20:19
@0xBEEFCAF3 0xBEEFCAF3 mentioned this pull request Nov 21, 2022
Closed
@DanGould
Copy link
Contributor

If you write "fix #79" in the description the issues will be linked

@0xBEEFCAF3
Copy link
Contributor Author

@DanGould Cab you rerun the test job please?
It looks like the failure is unrelated to the changes in this PR

thread 'integration::test' panicked at 'called `Result::unwrap()` on an `Err` value: ConnectError { internal: ReadFile { file: "/tmp/.tmpMbMual/peer-tls.cert", error: Os { code: 2, kind: NotFound, message: "No such file or directory" } } }',

@0xBEEFCAF3
Copy link
Contributor Author

@DanGould ready for review

DanGould
DanGould reviewed Nov 21, 2022
View reviewed changes
Copy link
Contributor

@DanGould DanGould left a comment

Choose a reason for hiding this comment

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

Thanks for grabbing this chore and implementing it really well.

I suggest we change the option to a default. CARGO_LOG envvar is set on debug by default when you run a default build anyhow, and in production it will default to info level.

README.md Outdated
4. Run with `nolooking --conf nolooking.conf`
5. Visit Nolooking on [127.0.0.1](http://127.0.0.1:3000) and queue some bitcoin channels.
6. Generate the QR code, pay it or share it! Once a payjoin transaction has enough confirmations, your new lightning channels will be established and you can move your sats over the lightning nework!
4. (Optional) nolooking uses [env_logger](https://docs.rs/env_logger/latest/env_logger/) to log messages varying priority. Use `RUST_LOG` env var to set your log priority. Valid options are error, warn, info, debug, trace
Copy link
Contributor

Choose a reason for hiding this comment

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

On principle we avoid options whenever possible. env_logger affords us sane defaults: rust-cli/env_logger#47 (comment)

src/http.rs Outdated
@@ -81,7 +82,7 @@ async fn serve_public_file(path: &str) -> Result<Response<Body>, HttpError> {
}

async fn handle_pj(scheduler: Scheduler, req: Request<Body>) -> Result<Response<Body>, HttpError> {
dbg!(req.uri().query());
info!("{:?}", req.uri().query());
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: If display isn't implemented, this is probably debug, not info

@DanGould DanGould merged commit 47de88f into payjoin:master Nov 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DanGould DanGould DanGould left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add logging with log crate
2 participants
@0xBEEFCAF3 @DanGould