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 traceImportant and traceError with formats #1986

Merged
merged 2 commits into from
Jun 9, 2018

Conversation

BlythMeister
Copy link
Contributor

trace and log already have these, but important and error do not.

@matthid
Copy link
Member

matthid commented Jun 7, 2018

I think this module has a particular nasty naming. Nobody knows what the difference between traceFAKE trace or traceImportant is

@BlythMeister
Copy link
Contributor Author

What is the difference lol

let traceErrorfn fmt = Printf.ksprintf traceError fmt

/// Writes an error message to stderr (in red) and without a line break
let traceErrorf fmt = Printf.ksprintf (fun text -> CoreTracing.postMessage (TraceData.ErrorMessage(text))) fmt
Copy link
Member

Choose a reason for hiding this comment

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

Actually I think we need to leave out the f variants as they shouldn't print a newline character. I think your implementation will

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I copied the method from other f variants...happy to remove them though, I don't get the use case of the f methods

Copy link
Member

Choose a reason for hiding this comment

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

It is the , false parameter - which doesn't exist for those types for some reason.

I don't get the use case of the f methods

One use case is to realise some prompt to the user like
Please enter value for X: | and have the cursor sitting at | instead of the next line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh now it makes perfect sense!
I can't think why someone would need important or error version anyways then. So I'll remove.

@BlythMeister
Copy link
Contributor Author

Maybe we could obsolete traceFAKE stating to use traceimportantfn ?

@matthid
Copy link
Member

matthid commented Jun 7, 2018

What is the difference lol

have to look it up every time. Some go to stderr and some to stdout and different colors.

@BlythMeister
Copy link
Contributor Author

Ahh right
Maybe we should add more info in docs?

@BlythMeister
Copy link
Contributor Author

@matthid is this ok to make it in now?

@matthid
Copy link
Member

matthid commented Jun 9, 2018

Yes, thanks!

@matthid matthid merged commit 2e89b23 into fsprojects:release/next Jun 9, 2018
@BlythMeister BlythMeister deleted the logging branch May 11, 2021 15:58
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