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

Fix feedback levels #10612

Merged
merged 4 commits into from
Sep 4, 2019
Merged

Fix feedback levels #10612

merged 4 commits into from
Sep 4, 2019

Conversation

maximedenes
Copy link
Member

We make sure that msg_notice is used for command-like queries (Print, Search,...), as documented in the Feedback API.

This is useful for IDEs, see e.g. coq-community/vscoq-legacy#35

@gares
Copy link
Member

gares commented Aug 2, 2019

Should we we kill msg_info? What is the legitimate use case? (wondering about making its type different, to avoid confusion)

@maximedenes
Copy link
Member Author

Should we we kill msg_info?

I don't think so.

What is the legitimate use case?

The one described in the API, that is information meant to be displayed in verbose mode (typically when running coqtop).

(wondering about making its type different, to avoid confusion)

The two functions are essentially doing the same thing, but correspond to different degrees of verbosity. I don't see what distinguishing them by typing would mean, TBH.

@gares
Copy link
Member

gares commented Aug 5, 2019

The two functions are essentially doing the same thing, but correspond to different degrees of verbosity. I don't see what distinguishing them by typing would mean, TBH.

My impression is that one of the two is used rarely, so maybe a single function with a ?if_verbose flag so that, in case you don't know, you pass the default value.

Anyway, the few examples of msg_notice you gave me (in a private chat) were variants of "foo is defined" that is not very useful, hence my questioning the entire thing.

Copy link
Member

@ejgallego ejgallego left a comment

Choose a reason for hiding this comment

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

Maybe the debug output in auto and even in ltac_profile should be priority "debug", certainly at most Info not Notice.

@maximedenes
Copy link
Member Author

Maybe the debug output in auto and even in ltac_profile should be priority "debug", certainly at most Info not Notice.

You mean the output of debug auto? I thought they were not Coq debug info, but rather a query to help users debug their scripts.

As per documentation, "info" essentially means that it will be displayed by coqtop's verbose mode only. Why wouldn't IDEs want to show the result of the ltac profiler?

@ejgallego
Copy link
Member

Indeed I guess that having both notice and info is a bit confusing, OK then for notice in this case.

Ultimately trace information seems specialized enough as to have it's own request.

@ejgallego
Copy link
Member

Indeed I guess that having both notice and info is a bit confusing

Most protocols tend to have a single information notification, however depending how you set the verbose level in the server more messages are generated then. This is normal as otherwise now all the info call are wasteful in the sense that they shouldn't run in the first place.

@maximedenes
Copy link
Member Author

Ultimately trace information seems specialized enough as to have it's own request.

I agree with that, but for now it seems a "notice" is the closest indeed.

Most protocols tend to have a single information notification, however depending how you set the verbose level in the server more messages are generated then.

I also agree. It is sometimes done this way today, but via ad-hoc guards to printer calls (testing if we are in verbose/debug mode, etc). It should certainly be consolidated, but it is out of scope for this modest PR.

@maximedenes
Copy link
Member Author

Any takers? @ejgallego maybe?

@maximedenes
Copy link
Member Author

@gares can you take this one?

@ejgallego ejgallego self-assigned this Sep 4, 2019
@ejgallego ejgallego added kind: fix This fixes a bug or incorrect documentation. kind: user messages Error messages, warnings, etc. labels Sep 4, 2019
@ejgallego ejgallego added this to the 8.10.0 milestone Sep 4, 2019
ejgallego added a commit that referenced this pull request Sep 4, 2019
Ack-by: ejgallego
Reviewed-by: gares
@ejgallego ejgallego merged commit 2d40331 into coq:master Sep 4, 2019
vbgl added a commit to vbgl/coq that referenced this pull request Sep 11, 2019
@maximedenes maximedenes deleted the fix-notices branch August 27, 2023 10:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: fix This fixes a bug or incorrect documentation. kind: user messages Error messages, warnings, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants