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

feat: add #help note command #948

Open
wants to merge 23 commits into
base: main
Choose a base branch
from

Conversation

edegeltje
Copy link

@edegeltje edegeltje commented Sep 14, 2024

on zulip it was mentioned that there used to be an easy way to find and/or read library notes, but that this feature no longer works, presumably since lean4.

This PR aims to remedy the situation by introducing the #help note "some tag" command, which displays all library notes marked with the tag "some tag" that are declared before the command, including in imports.

@edegeltje edegeltje changed the title Add check_note command feat:Add check_note command Sep 14, 2024
@edegeltje
Copy link
Author

awaiting-review

@github-actions github-actions bot added the awaiting-review This PR is ready for review; the author thinks it is ready to be merged. label Sep 20, 2024
@leanprover-community-bot
Copy link
Collaborator

leanprover-community-bot commented Sep 27, 2024

Mathlib CI status (docs):

@fgdorais
Copy link
Collaborator

fgdorais commented Sep 27, 2024

The Mathlib test help_cmd.lean will need to be updated as a result of this. Was this expected?

PS: Thank you for being the first live test of this new Batteries feature that checks whether a Batteries PR will break Mathlib downstream!

@fgdorais fgdorais changed the title feat:Add check_note command feat: add #check_note command Sep 28, 2024
@edegeltje
Copy link
Author

Was this expected?

when writing this PR i certainly did not take this in consideration, so it was not something i was expecting...
still, looking at how the failing test is written, it is not a surprise it "broke", as its success does indeed depend on what commands start with "#chec" despite that not being the goal of the test.

leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Sep 28, 2024
@fgdorais
Copy link
Collaborator

fgdorais commented Sep 28, 2024

Indeed, that is a poorly designed test. On the other hand, that test's output shows that #check_notes doesn't quite fit with the other #check commands. It fits better with #print commands but not perfectly. Perhaps it should be a #help command? What do you think?

@edegeltje
Copy link
Author

edegeltje commented Sep 28, 2024

i guess it could be a #help command, but i think #help is in mathlib? i think #show_note is an acceptable alternative name. What mathlib does with #help is their concern, they are free to reuse the code here to get the output, although i don't think this feature fits the expectations of #help...

@fgdorais
Copy link
Collaborator

Actually, it makes sense to move #help here or even to Lean since it's useful to everyone and nothing about it is Mathlib specific. 🤔

@fgdorais
Copy link
Collaborator

The #help command is now in Batteries. This PR can now be adapted as the #help notes subcommand.

@edegeltje edegeltje changed the title feat: add #check_note command feat: add #help note command Oct 16, 2024
@edegeltje
Copy link
Author

wip

@github-actions github-actions bot added WIP work in progress and removed awaiting-review This PR is ready for review; the author thinks it is ready to be merged. labels Oct 16, 2024
@edegeltje
Copy link
Author

awaiting-review

@github-actions github-actions bot removed the WIP work in progress label Oct 16, 2024
@github-actions github-actions bot added the awaiting-review This PR is ready for review; the author thinks it is ready to be merged. label Oct 16, 2024
@edegeltje
Copy link
Author

edegeltje commented Oct 16, 2024

unfortunately it seems i can't write a test to make sure the command indeed does work across imports, as i can't figure out how to import a test file in another test file, and writing a library note in the actual module for the sole purpose of testing might not be desirable. In particular, it is impossible to write a library note in the same file that the environment extension is defined.

@fgdorais
Copy link
Collaborator

This is what Batteries.Test.Internal is for! You can add DummyLibraryNote.lean in that folder and import it in the test file.

@edegeltje
Copy link
Author

WIP

@github-actions github-actions bot added WIP work in progress and removed awaiting-review This PR is ready for review; the author thinks it is ready to be merged. labels Oct 16, 2024
@edegeltje
Copy link
Author

awaiting-review

@github-actions github-actions bot added awaiting-review This PR is ready for review; the author thinks it is ready to be merged. and removed WIP work in progress labels Oct 16, 2024
@fgdorais
Copy link
Collaborator

You need to add the dummy files to Batteries.lean.

This looks really good! Other #help subcommands support prefix search, I don't think that's essential but it would be nice.

I'll be busy for the rest of the day, so I'll come back for a review tomorrow.

@edegeltje
Copy link
Author

edegeltje commented Oct 16, 2024

i added code for prefix search, but left it in comments since it depends on List.ne_nil_of_mem_groupBy (substituted with sorry) from mathlib... if the lemma could get upstreamed, we can enable the feature (and add appropriate tests)

@fgdorais
Copy link
Collaborator

You can use List.head! instead of List.head.

@edegeltje
Copy link
Author

ok, it should have all the desired features now. please make sure the documentation and tests are sufficient.

Batteries/Util/LibraryNote.lean Outdated Show resolved Hide resolved
Batteries/Util/LibraryNote.lean Outdated Show resolved Hide resolved
Batteries/Util/LibraryNote.lean Outdated Show resolved Hide resolved

/--
info: library_note "Other"
/--
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see the utility of wrapping each note in /-- ... -/. How about just separating with a blank line or ---?

Copy link
Author

Choose a reason for hiding this comment

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

i think the benefit is that this is less likely to cause confusion as to where one note stops and another begins, and this mimics the way you'd find the note in the actual code.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess my issue is that /-- ... -/ has a specific meaning in Lean, which is not relevant here. So, for example, copy pasting the output in a Lean file would lead to multiple errors. How about /- ... -/? (No errors then, though that still seems visually cluttered.)

Copy link
Member

Choose a reason for hiding this comment

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

We aren't doing this for any of the other #help commands, I think the docs should just be indented relative to the library_note header.

Copy link
Collaborator

@fgdorais fgdorais Oct 18, 2024

Choose a reason for hiding this comment

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

I reflected on this a bit and I think #help subcommands should give output that is better than plain code lookup in some useful way. In many cases organization is the key difference but formatting is relevant too. I don't know what would be the best formatting for library notes. How about asking on Mathlib Zulip what users would like to see for library notes?

@fgdorais
Copy link
Collaborator

fgdorais commented Oct 19, 2024

By the way, no worries about the current breaks-mathlib label. That will be fixed downstream once mathlib4#17257 gets merged.

@edegeltje
Copy link
Author

edegeltje commented Oct 20, 2024

By the way, no worries about the current breaks-mathlib label. That will be fixed downstream once mathlib4#17257 gets merged.

actually, it looks to me like the script trying to find the mathlib branch fails...
see for example how this run fails with an error trying to get info about the PR triggering the ci, from what i can tell... so even before looking at mathlib

@fgdorais
Copy link
Collaborator

Yes, the script is broken right now, we're doing manual testing for the time being.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-review This PR is ready for review; the author thinks it is ready to be merged. breaks-mathlib
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants