Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[Merged by Bors] - refactor(library/init/meta/*): refactor case tags #228
[Merged by Bors] - refactor(library/init/meta/*): refactor case tags #228
Changes from all commits
ce6b249
1e48484
3c71ed4
d796c32
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also add
has_repr
,has_to_tactic_format
, andhas_format
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There seems to be no instance
has_repr name
(which blockshas_repr case_tag
). Is this intentional?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the intention behind the
repr
/to_string
split is thatrepr
should output "valid" Lean code, andto_string
a "Human-readable" version. But we are missing lots of instances because of this stringification class inflation. Practically speaking, it's really frustrating even if only some of the instances are missing because you need all of them. For example,#eval
wanthas_repr
,pp
wantshas_to_tactic_format
, etc.So yes, please add
has_repr name
. And addhas_repr case_tag
no matter whether there is ahas_repr name
or not. (I would have been ok withinstance : has_repr case_tag := ⟨to_string⟩
.)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get the
repr
/to_string
split; there's a semantic difference there. What I think is unnecessary ishas_format
/to_string
. Ifformat
wasn'tmeta
-- and I don't see why it should be --to_string
could be subsumed byhas_format
.Anyway, I'll add the
has_repr
instances.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I hit the merge button a bit too early.
BTW,
to_string
is not subsumed byto_fmt
. There is nohas_to_string
instance usinghas_to_format
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, my point was that
to_string
could be subsumed byto_fmt
ifhas_format
wasn'tmeta
, becauseto_format
andto_string
serve the same purpose (human-friendly printing).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 for adding a low-priority instance converting from
has_to_format
tohas_to_string
.