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: remove porting notes about the nonempty instance linter #20824

Closed
wants to merge 3 commits into from

Conversation

kim-em
Copy link
Contributor

@kim-em kim-em commented Jan 18, 2025

@kim-em
Copy link
Contributor Author

kim-em commented Jan 18, 2025

Closes #5171

Copy link

github-actions bot commented Jan 18, 2025

PR summary 8e5b5529fd

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/declarations_diff.sh <optional_commit>

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

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


Decrease in tech debt: (relative, absolute) = (162.00, 0.04)
Current number Change Type
4406 -162 porting notes

Current commit 8e5b5529fd
Reference commit 3bf4857ada

You can run this locally as

./scripts/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).

@grunweg
Copy link
Collaborator

grunweg commented Jan 18, 2025

The diff does what it says. Can you take a step back and tell us why this is fine to remove?

  • Is there documentation what the linter did in mathlib3? Is that functionality incorporated into Lean core/ unnecessary somehow?
    (If not, a better fix seems to be porting the linter to mathlib4. Is there are instructions what it does, I could try this myself.)
  • If the linter is going to be re-porting, having those comments could be helpful, right? (At least, manually having to add this attribute 200 times will be annoying.) Or is your argument that one should re-audit all those locations anyway, so removing those notes now is fine?

@grunweg grunweg added awaiting-author A reviewer has asked the author a question or requested changes porting-notes Mathlib3 to Mathlib4 porting notes. tech debt Tracking cross-cutting technical debt, see e.g. the "Technical debt counters" stream on zulip labels Jan 18, 2025
@bryangingechen
Copy link
Contributor

(I've added the Zulip link to the PR description / eventual commit message.)

@grunweg
Copy link
Collaborator

grunweg commented Jan 18, 2025

Thanks! Makes much more sense now.

@leanprover-community-bot-assistant leanprover-community-bot-assistant added the merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) label Jan 18, 2025
@jcommelin
Copy link
Member

bors d+

@mathlib-bors
Copy link
Contributor

mathlib-bors bot commented Jan 20, 2025

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

@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added delegated and removed awaiting-author A reviewer has asked the author a question or requested changes labels Jan 20, 2025
@kim-em
Copy link
Contributor Author

kim-em commented Jan 20, 2025

bors merge

@mathlib-bors
Copy link
Contributor

mathlib-bors bot commented Jan 20, 2025

👎 Rejected by label

@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added the ready-to-merge This PR has been sent to bors. label Jan 20, 2025
@leanprover-community-bot-assistant leanprover-community-bot-assistant removed the merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) label Jan 20, 2025
@kim-em
Copy link
Contributor Author

kim-em commented Jan 21, 2025

bors merge

@mathlib-bors
Copy link
Contributor

mathlib-bors bot commented Jan 21, 2025

Pull request successfully merged into master.

Build succeeded:

@mathlib-bors mathlib-bors bot changed the title chore: remove porting notes about the nonempty instance linter [Merged by Bors] - chore: remove porting notes about the nonempty instance linter Jan 21, 2025
@mathlib-bors mathlib-bors bot closed this Jan 21, 2025
@mathlib-bors mathlib-bors bot deleted the nonempty_instance_porting_notes branch January 21, 2025 04:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
delegated porting-notes Mathlib3 to Mathlib4 porting notes. ready-to-merge This PR has been sent to bors. tech debt Tracking cross-cutting technical debt, see e.g. the "Technical debt counters" stream on zulip
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants