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: make MissingDocs extensible #1414

Closed
wants to merge 4 commits into from

Conversation

digama0
Copy link
Collaborator

@digama0 digama0 commented Aug 3, 2022

This should also fix the compile time cost issue noticed in #1390. This adds an environment extension and attribute to add MissingDocs.Handlers for user-defined commands, so that they can also be docs linted. The only thing you have to do in a handler is identify if there is a docstring on the syntax, and if not then call the lintNamed function with the span of the new identifier.

You can implement either Handler or SimpleHandler:

  • SimpleHandler := Syntax -> CommandElabM Unit gets the syntax of the command and possibly reports a lint warning. It is only called if the syntax has the syntax kind specified in the attribute, and if the missingDocs linter itself is enabled.
  • Handler := Bool -> Syntax -> CommandElabM Unit is called even if the linter is disabled, and is passed the enabled state. This is needed for commands that contain other commands, because nested commands can have a set_option that locally re-enables the linter.

Comment on lines +23 to +24
abbrev SimpleHandler := Syntax → CommandElabM Unit
abbrev Handler := Bool → SimpleHandler
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
abbrev SimpleHandler := Syntax → CommandElabM Unit
abbrev Handler := Bool → SimpleHandler
abbrev SimpleHandler ks := TSyntax ks → CommandElabM Unit
abbrev Handler ks := Bool → SimpleHandler ks

Then we could also drop the foo in @[missingDocsHandler foo].

Copy link
Collaborator Author

@digama0 digama0 Aug 3, 2022

Choose a reason for hiding this comment

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

That makes the evalConst a lot trickier, I think? EDIT: Yeah, I don't want to deal with this. This can be a separate PR, and I probably won't be the one to write it. I think it's already usable enough for practical purposes, and it accomplishes the stated goal of extensibility to declarations outside the core.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, this is an interesting general refactoring that we can apply to other places as well, especially after strengthening the macro typing story.

@leodemoura
Copy link
Member

Merged manually due to stage0 conflicts

@leodemoura leodemoura closed this Aug 5, 2022
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.

4 participants