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

chore(Data/List/Basic): split take and drop from file #21232

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

BoltonBailey
Copy link
Collaborator

@BoltonBailey BoltonBailey commented Jan 29, 2025

This PR splits Data/List/Basic.lean decls and lemmas related to take or drop into a new TakeDrop.lean file, decreasing the length of a file triggering the long file linter.


Open in Gitpod

Copy link

github-actions bot commented Jan 29, 2025

PR summary 848261948d

Import changes for modified files

Dependency changes

File Base Count Head Count Change
Mathlib.Data.Array.Lemmas 366 362 -4 (-1.09%)
Import changes for all files
Files Import difference
There are 4102 files with changed transitive imports taking up over 178422 characters: this is too many to display!
You can run scripts/import_trans_difference.sh all locally to see the whole output.

Declarations diff

+ splitAt_eq_take_drop

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.


No changes to technical debt.

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

@BoltonBailey BoltonBailey added the tech debt Tracking cross-cutting technical debt, see e.g. the "Technical debt counters" stream on zulip label Jan 29, 2025
Copy link
Contributor

@Vierkantor Vierkantor left a comment

Choose a reason for hiding this comment

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

Nice work overall, the split makes sense although it's unfortunate it only ends up reducing the total number of lines by 10%. We still have some work to do!

A couple suggestions to improve the import graph even more.

Mathlib/Data/List/TakeDrop.lean Outdated Show resolved Hide resolved
scripts/noshake.json Outdated Show resolved Hide resolved
Mathlib/Data/List/MinMax.lean Outdated Show resolved Hide resolved
Mathlib/Data/List/Nodup.lean Outdated Show resolved Hide resolved
@Vierkantor Vierkantor added the awaiting-author A reviewer has asked the author a question or requested changes label Jan 30, 2025
@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 Feb 1, 2025
@github-actions github-actions bot removed the merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) label Feb 6, 2025
@BoltonBailey BoltonBailey added awaiting-CI and removed awaiting-author A reviewer has asked the author a question or requested changes labels Feb 7, 2025
@BoltonBailey BoltonBailey changed the title chore(Data/List/Basic.lean): split take and drop from file chore(Data/List/Basic): split take and drop from file Feb 8, 2025
@BoltonBailey
Copy link
Collaborator Author

BoltonBailey commented Feb 8, 2025

This will collide with #21530

Frankly, that PR is much more extensive splitting and should be reviewed and merged first, possibly this PR can just be closed if so.

@kim-em
Copy link
Contributor

kim-em commented Feb 9, 2025

I didn't touch take and drop in the PR, so I'm hopeful we can just do both.

@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 Feb 10, 2025
@kim-em
Copy link
Contributor

kim-em commented Feb 10, 2025

I've merged master now, it will at least need a shake --fix still.

@github-actions github-actions bot removed the merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) label Feb 10, 2025
@kim-em
Copy link
Contributor

kim-em commented Feb 11, 2025

bors d+

@mathlib-bors
Copy link
Contributor

mathlib-bors bot commented Feb 11, 2025

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
delegated 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