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 set_option pp.explicit false in Weak Head Normal Form example #146

Merged
merged 2 commits into from
Sep 29, 2024

Conversation

kpadmasola
Copy link
Contributor

In the following example in Weak Head Normalisation section:

the expected output is:

#eval whnf' `(List.cons 1 [])
-- [1]

but the actual output is:

#eval whnf' `(List.cons 1 [])
-- @List.cons Nat 1 (@List.nil Nat)

This PR fixes the example code so that the actual output matches the one in the book.

@kpadmasola
Copy link
Contributor Author

@Julian Can you please review ? Thanks!

@@ -639,6 +639,7 @@ Now, here are some examples of expressions in WHNF.
Constructor applications are in WHNF (with some exceptions for numeric types):
-/

set_option pp.explicit false
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the real issue here is above on line 569, and specifically that that line should have been doing

set_option pp.explicit true in

(with the in) rather than globally changing the setting?

Could you perhaps try that instead and check whether that makes everything look correct?

Meanwhile, as someone who doesn't know much at all about metaprogramming, I find this example confusing to start off with -- the text just got finished explaining what the general form of something in WHNF is, and then here immediately starts with an example that prints differently? Naively to me it definitely seems worthwhile to explain that the pretty printer can make something look not WHNF even when it is, but starting with such an example seems weird if I follow what the text is saying.

But it does seem like the intention is to show [1] and that that's what it used to do before the change to 569, so I'm happy to merge if you take a look at the in bit above?

And thanks as usual!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Julian The change you suggested worked, thanks! PR updated.

@kpadmasola kpadmasola requested a review from Julian September 29, 2024 17:04
@Julian Julian merged commit 7246ae1 into leanprover-community:master Sep 29, 2024
5 checks passed
@kpadmasola kpadmasola deleted the MetaM-minor-fixes branch September 30, 2024 00:35
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