Skip to content

int: replace the (e)println! with log#51

Closed
JiangengDong wants to merge 3 commits intosequenceplanner:masterfrom
JiangengDong:jiangeng/replace_print_with_log
Closed

int: replace the (e)println! with log#51
JiangengDong wants to merge 3 commits intosequenceplanner:masterfrom
JiangengDong:jiangeng/replace_print_with_log

Conversation

@JiangengDong
Copy link

Currently, r2r prints error messages with println! and eprintln!. However, this is not good for debugging, because this is no call site information, and the (e)println! is hard to be silent.

In this PR, all the println! are replaced with log::debug! and all the eprintln! are replaced with log::error!. This gives the users more flexibility and better control over the output and makes it easier to locate the problem.

@JiangengDong
Copy link
Author

Well, I accidentally also changed the println! in mod tests and in comments. I am going to push a commit to revert them later.

@m-dahl
Copy link
Collaborator

m-dahl commented Jun 2, 2023

I feel kind of bad that you are doing all the TODOs that never got done :). This is a really needed pr 👍 Just me know when you are ready to to merge it.

@JiangengDong
Copy link
Author

Pushed a commit to revert the println! used in tests and comments. Sorry, I have been busy last week with my regular work.

Should this be a minor version change or a patch version change?

@JiangengDong JiangengDong force-pushed the jiangeng/replace_print_with_log branch from 0dd6150 to 7c1d8d5 Compare June 10, 2023 20:44
@m-dahl
Copy link
Collaborator

m-dahl commented Jun 11, 2023

No worries. Unless there is something more you would like to do in this PR we can just merge this for now, and I am thinking we should make a release that bumps to 0.8 once we make the change static array change.

@JiangengDong JiangengDong force-pushed the jiangeng/replace_print_with_log branch from 9ecc51a to aad057a Compare June 25, 2023 17:57
@JiangengDong
Copy link
Author

Rebased to the latest master branch, and pushed new commits to increase the version to 0.7.4.

I think we can merge this if there is no other concern.

@m-dahl
Copy link
Collaborator

m-dahl commented Jun 25, 2023

I just merged this but I didn't include the version bump since I don't have time to make a release just now.

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.

2 participants