Skip to content

[Merged by Bors] - chore: reorder arguments in DirSupClosed#37277

Closed
vihdzp wants to merge 5 commits intoleanprover-community:masterfrom
vihdzp:dirsupclosed2
Closed

[Merged by Bors] - chore: reorder arguments in DirSupClosed#37277
vihdzp wants to merge 5 commits intoleanprover-community:masterfrom
vihdzp:dirsupclosed2

Conversation

@vihdzp
Copy link
Copy Markdown
Collaborator

@vihdzp vihdzp commented Mar 27, 2026

We move d ⊆ s out of an unrelated binder.


Open in Gitpod

@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 27, 2026

PR summary 251d9c0ff4

Import changes for modified files

No significant changes to the import graph

Import changes for all files
Files Import difference

Declarations diff

No declarations were harmed in the making of this PR! 🐙

You can run this locally as follows
## summary with just the declaration names:
./scripts/pr_summary/declarations_diff.sh <optional_commit>

## more verbose report:
./scripts/pr_summary/declarations_diff.sh long <optional_commit>

The doc-module for scripts/pr_summary/declarations_diff.sh contains some details about this script.


No changes to technical debt.

You can run this locally as

./scripts/reporting/technical-debt-metrics.sh pr_summary
  • The relative value is the weighted sum of the differences with weight given by the inverse of the current value of the statistic.
  • The absolute value is the relative value divided by the total sum of the inverses of the current values (i.e. the weighted average of the differences).

@mathlib-dependent-issues mathlib-dependent-issues bot added the blocked-by-other-PR This PR depends on another PR (this label is automatically managed by a bot) label Mar 27, 2026
@vihdzp vihdzp added the t-order Order theory label Mar 27, 2026
@vihdzp vihdzp requested a review from b-mehta March 27, 2026 17:11
@mathlib-dependent-issues mathlib-dependent-issues bot removed the blocked-by-other-PR This PR depends on another PR (this label is automatically managed by a bot) label Mar 27, 2026
@mathlib-dependent-issues
Copy link
Copy Markdown

This PR/issue depends on:

@Vierkantor
Copy link
Copy Markdown
Contributor

I'm going to (weakly) push back on this change. Even though I agree with you: of the two versions, the new one is better code. It's because I don't see that this makes things better enough. Is there a proof in a downstream project that becomes noticeably easier when you change this? And if that is the case, this PR is not enough, I think we should not change only one def: can we turn this into a general code style guideline, write a linter (optional but would be very good to have) and then make the improvement everywhere in Mathlib?

@Vierkantor Vierkantor added the awaiting-author A reviewer has asked the author a question or requested changes. label Apr 2, 2026
@vihdzp
Copy link
Copy Markdown
Collaborator Author

vihdzp commented Apr 2, 2026

I wouldn't say this makes the proofs easier, but I do think it makes writing them easier. The binders are in a more predictable order, and I it means I don't have to introduce four other variables before being reminded "ah! d is a subset of s".

If we want to turn this into a guideline, I'd imagine it be saying that h → ∀ a, p a is the preferred spelling of ∀ a, h → p a.

@Vierkantor
Copy link
Copy Markdown
Contributor

If we want to turn this into a guideline, I'd imagine it be saying that h → ∀ a, p a is the preferred spelling of ∀ a, h → p a.

Agreed! I think we have a simproc even that does this rewrite. So if you could add this rule to the review checklist (or similar) then I'll approve it and we can look together at doing this in more places in Mathlib.

@b-mehta
Copy link
Copy Markdown
Contributor

b-mehta commented Apr 2, 2026

I agree we should make this change globally too, but perhaps for pragmatic reasons (and since there's a few PRs around Scott and ScottHausdorff I'd like to prioritise right now), @Vierkantor do you think we could merge this one and do the broader changes in another PR?
(I also suspect there won't be many changes like this to make!)

@Vierkantor
Copy link
Copy Markdown
Contributor

Okay! It was only weak pushback, so if you feel it is valuable in this form then I retract my objection. :)

bors r+

@mathlib-triage mathlib-triage bot added ready-to-merge This PR has been sent to bors. and removed awaiting-author A reviewer has asked the author a question or requested changes. labels Apr 2, 2026
mathlib-bors bot pushed a commit that referenced this pull request Apr 2, 2026
We move `d ⊆ s` out of an unrelated binder.
@mathlib-bors
Copy link
Copy Markdown
Contributor

mathlib-bors bot commented Apr 2, 2026

Build failed (retrying...):

mathlib-bors bot pushed a commit that referenced this pull request Apr 2, 2026
We move `d ⊆ s` out of an unrelated binder.
@mathlib-bors
Copy link
Copy Markdown
Contributor

mathlib-bors bot commented Apr 2, 2026

Pull request successfully merged into master.

Build succeeded:

@mathlib-bors mathlib-bors bot changed the title chore: reorder arguments in DirSupClosed [Merged by Bors] - chore: reorder arguments in DirSupClosed Apr 2, 2026
@mathlib-bors mathlib-bors bot closed this Apr 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-to-merge This PR has been sent to bors. t-order Order theory

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants