Skip to content

feat(blueprint.yml): run environment linters also #427

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
merged 9 commits into from
Apr 26, 2025

Conversation

grunweg
Copy link
Contributor

@grunweg grunweg commented Apr 22, 2025

Completes the remaining item of #418, so closes #418.

mathlib's style.longLine linter checks the same already
@kbuzzard
Copy link
Collaborator

CI was passing with PRs with long lines even before you proposed deleting the line which was supposed to be catching them?

@grunweg
Copy link
Contributor Author

grunweg commented Apr 22, 2025

Well, as I see it

  • the grep step was supposed to catch lines longer than 100 characters --- but didn't quite work, since there were longer lines
  • the syntax linter step also catches long lines: but it works wherever it is imported. (Ensuring the flexible linter is imported everywhere, as your last PR did, suffices.)
  • Thus, both steps should have worked the same --- except that in practice, the grep check didn't work.

@grunweg
Copy link
Contributor Author

grunweg commented Apr 22, 2025

Let me fix the linter invocation, just a moment.

This should catch unnecessary haves, def/lemma confusions and other errors.
@grunweg
Copy link
Contributor Author

grunweg commented Apr 22, 2025

(You need to re-click the "approve workflow" button: github wants to prevent me from mining cryptocurrency on your CI, presumably against your will. I approve of the goal, but that means extra work for you...)

@grunweg
Copy link
Contributor Author

grunweg commented Apr 23, 2025

I added the mk_all --check step to CI as well, which pointed out that something was missing. #430 updates the file.

@grunweg
Copy link
Contributor Author

grunweg commented Apr 23, 2025

The CI step works - the workflow fails because there are a couple of linter errors to fix :-)

@grunweg
Copy link
Contributor Author

grunweg commented Apr 23, 2025

Update: this PR can be merged once all pre-requisites have landed:
#430 will fix FLT.lean; #427 will fix the environment linter warnings (or nolint them), then this PR should should pass CI.

@grunweg
Copy link
Contributor Author

grunweg commented Apr 23, 2025

(There is of course a policy question: should one mandate new PRs adding doc-strings to all definitions? I'll leave that to you to decide.)

@grunweg grunweg mentioned this pull request Apr 23, 2025
5 tasks
@kbuzzard
Copy link
Collaborator

(There is of course a policy question: should one mandate new PRs adding doc-strings to all definitions? I'll leave that to you to decide.)

Yes I would like to do that! How do I do that?

@kbuzzard
Copy link
Collaborator

NB I won't be closing #418 just yet -- I am now basically using it as a tracking issue for linters which I might want.

@kbuzzard
Copy link
Collaborator

OK so I have rerun CI and there are a ton of errors. I will start on the definition docstrings. What do I do about #431 ? Should that be merged into this?

@kbuzzard
Copy link
Collaborator

kbuzzard commented Apr 26, 2025

Oh I see -- I can just merge #431 independently, and also I can merge definition docstrings independently, and we can keep running CI on this until it passes? 💡

@kbuzzard
Copy link
Collaborator

@grunweg I don't understand the workflow here. #431 has added this huge scripts/nolints.json file to the project, which makes me feel guilty, so now I made #437 adding some docstrings to definitions, and now this huge file needs to be edited right? How did you create it? IIRC in mathlib we occasionally get auto-generated PRs from Gabriel saying "let me remove some nolints for you". I'm not suggesting we need this but I guess it would be nice to know how you made the file, so I can remake it every time I fix some of the remaining issues.

@kbuzzard kbuzzard mentioned this pull request Apr 26, 2025
@kbuzzard
Copy link
Collaborator

Hopefully #439 is all that's left.

@kbuzzard kbuzzard merged commit 539d2a7 into ImperialCollegeLondon:main Apr 26, 2025
2 checks passed
@kbuzzard
Copy link
Collaborator

Once again, thanks so much!

@grunweg
Copy link
Contributor Author

grunweg commented Apr 27, 2025

You're very welcome!

@grunweg grunweg deleted the MR-linters branch April 27, 2025 14:45
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.

linters I would like
2 participants