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

[Merged by Bors] - chore: adapt to multiple goal linter 1 #12338

Closed
wants to merge 10 commits into from

Conversation

adomani
Copy link
Collaborator

@adomani adomani commented Apr 22, 2024

A PR accompanying #12339.

Zulip discussion


Open in Gitpod

Copy link
Collaborator

@grunweg grunweg left a comment

Choose a reason for hiding this comment

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

You didn't mark this ready for review yet. I still decided to leave a few comments; hope they helpful.

To my eye, most of these look like clear improvements that should be uncontroversial. I commented on other that might require more thought or where follow-up golfing is possible.

@@ -355,7 +355,7 @@ def gluedCover : Scheme.GlueData.{u} where
This is an isomorphism, as witnessed by an `IsIso` instance. -/
def fromGlued : 𝒰.gluedCover.glued ⟶ X := by
fapply Multicoequalizer.desc
exact fun x => 𝒰.map x
· exact fun x => 𝒰.map x
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you move this into the fapply? If possible, I'd find this even nicer.

Mathlib/Data/Seq/Parallel.lean Show resolved Hide resolved
Mathlib/Data/Seq/Parallel.lean Show resolved Hide resolved
Mathlib/Data/Seq/Seq.lean Show resolved Hide resolved
Mathlib/AlgebraicGeometry/Gluing.lean Outdated Show resolved Hide resolved
Mathlib/Algebra/Order/Group/Abs.lean Outdated Show resolved Hide resolved
Mathlib/Data/Int/Bitwise.lean Outdated Show resolved Hide resolved
Mathlib/Data/List/Permutation.lean Outdated Show resolved Hide resolved
Mathlib/Data/List/Zip.lean Show resolved Hide resolved
exact ⟨fun h => Option.noConfusion h, fun h => Option.noConfusion h.2⟩
repeat' erw [WithBot.coe_eq_coe]
exact add_eq_zero_iff' (zero_le _) (zero_le _)
repeat (· exact ⟨fun h => Option.noConfusion h, fun h => Option.noConfusion h.1⟩)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm on the fence if the any_goals => repeat change is better.

@adomani adomani added the awaiting-review The author would like community review of the PR label Apr 23, 2024
Comment on lines +194 to +196
rw [List.eq_replicate.2 ⟨_, h⟩, prod_replicate, one_pow]
· exact (length l)
· rfl⟩
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
rw [List.eq_replicate.2 ⟨_, h⟩, prod_replicate, one_pow]
· exact (length l)
· rfl⟩
rw [List.eq_replicate.mpr ⟨rfl, h⟩, prod_replicate, one_pow]⟩

@@ -129,7 +129,7 @@ theorem hasFDerivAt_integral_of_dominated_loc_of_lip' {F' : α → H →L[𝕜]
exact ((hF_meas _ x_in).sub (hF_meas _ x₀_in)).sub (hF'_meas.apply_continuousLinearMap _)
· refine mem_of_superset h_ball fun x hx ↦ ?_
apply (h_diff.and h_lipsch).mono
rintro a ⟨-, ha_bound⟩
on_goal 1 => rintro a ⟨-, ha_bound⟩
Copy link
Collaborator

Choose a reason for hiding this comment

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

I get why this was linted this way, but it seems unnecessary. The 2nd goal is just a bounding function waiting to get described by the next line.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have been using on_goal sometimes as a place-holder for something where maybe the linter should be manually silenced. I'll leave this comment visible, in case someone else wants to chime in.

Comment on lines +80 to +81
on_goal 1 => let b : Finset ℂ := ?_
on_goal 1 => let c : Finset ℂ := ?_
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps the linter shouldn't add on_goal when the additional goals are terms of Type and not Prop.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is probably a good heuristic, though I would limit the use of this heuristic to theorems. Again, I'll leave this up for further comments.

@@ -649,15 +649,15 @@ theorem cont_eval_fix {f k v} (fok : Code.Ok f) :
obtain ⟨v'', h₁, h₂⟩ := this
rw [reaches_eval] at h₂
swap
exact ReflTransGen.single rfl
· exact ReflTransGen.single rfl
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could be on_goal 2 => exact ReflTransGen.single rfl removing the previous swap.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tried to recycle as much as possible the original syntax. I would prefer this change to be a separate PR.

@adomani
Copy link
Collaborator Author

adomani commented Apr 23, 2024

!bench

@leanprover-bot
Copy link
Collaborator

Here are the benchmark results for commit 68a02d1.
There were no significant changes against commit 5b96848.

@adomani adomani changed the title chore: a batch of cdot usage chore: adapt to multiple goal linter 1 Apr 23, 2024
Mathlib/Data/List/Basic.lean Outdated Show resolved Hide resolved
Co-authored-by: Ruben Van de Velde <65514131+Ruben-VandeVelde@users.noreply.github.com>
@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added the merge-conflict The PR has a merge conflict with master, and needs manual merging. label Apr 24, 2024
@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added merge-conflict The PR has a merge conflict with master, and needs manual merging. and removed merge-conflict The PR has a merge conflict with master, and needs manual merging. labels Apr 25, 2024
@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot removed the merge-conflict The PR has a merge conflict with master, and needs manual merging. label Apr 26, 2024
@jcommelin
Copy link
Member

bors d+

@mathlib-bors
Copy link

mathlib-bors bot commented Apr 30, 2024

✌️ adomani can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

@github-actions github-actions bot added delegated and removed awaiting-review The author would like community review of the PR labels Apr 30, 2024
@adomani
Copy link
Collaborator Author

adomani commented Apr 30, 2024

bors r+

@mathlib-bors
Copy link

mathlib-bors bot commented Apr 30, 2024

Pull request successfully merged into master.

Build succeeded:

@mathlib-bors mathlib-bors bot changed the title chore: adapt to multiple goal linter 1 [Merged by Bors] - chore: adapt to multiple goal linter 1 Apr 30, 2024
@mathlib-bors mathlib-bors bot closed this Apr 30, 2024
@mathlib-bors mathlib-bors bot deleted the adomani/use_cdots branch April 30, 2024 07:18
mathlib-bors bot pushed a commit that referenced this pull request Apr 30, 2024
A PR analogous to #12338 and #12361: reformatting proofs following the multiple goals linter of #12339.
mathlib-bors bot pushed a commit that referenced this pull request Apr 30, 2024
A PR analogous to #12338: reformatting proofs following the multiple goals linter of #12339.
mathlib-bors bot pushed a commit that referenced this pull request Apr 30, 2024
A PR analogous to #12338 and #12361: reformatting proofs following the multiple goals linter of #12339.
apnelson1 pushed a commit that referenced this pull request May 12, 2024
A PR analogous to #12338: reformatting proofs following the multiple goals linter of #12339.
apnelson1 pushed a commit that referenced this pull request May 12, 2024
A PR analogous to #12338 and #12361: reformatting proofs following the multiple goals linter of #12339.
mathlib-bors bot pushed a commit that referenced this pull request May 13, 2024
This PR was reduced by approximately half of its original size: the other half of this PR is now the content of #12560.

This PR was reduced by approximately half of its halved size: the other half of the halved PR is now the content of #12834.

A PR analogous to the merged PRs #12338, #12361 and #12372: reformatting proofs following the multiple goals linter of #12339.

This should be the last of the adaptations.
callesonne pushed a commit that referenced this pull request May 16, 2024
A PR analogous to #12338: reformatting proofs following the multiple goals linter of #12339.
callesonne pushed a commit that referenced this pull request May 16, 2024
A PR analogous to #12338 and #12361: reformatting proofs following the multiple goals linter of #12339.
callesonne pushed a commit that referenced this pull request May 16, 2024
This PR was reduced by approximately half of its original size: the other half of this PR is now the content of #12560.

This PR was reduced by approximately half of its halved size: the other half of the halved PR is now the content of #12834.

A PR analogous to the merged PRs #12338, #12361 and #12372: reformatting proofs following the multiple goals linter of #12339.

This should be the last of the adaptations.
grunweg pushed a commit that referenced this pull request May 17, 2024
This PR was reduced by approximately half of its original size: the other half of this PR is now the content of #12560.

This PR was reduced by approximately half of its halved size: the other half of the halved PR is now the content of #12834.

A PR analogous to the merged PRs #12338, #12361 and #12372: reformatting proofs following the multiple goals linter of #12339.

This should be the last of the adaptations.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants