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: rename fields of Subarray to follow Lean conventions #3851

Merged

Conversation

david-christiansen
Copy link
Contributor

No description provided.

@github-actions github-actions bot added the toolchain-available A toolchain is available for this PR, at leanprover/lean4-pr-releases:pr-release-NNNN label Apr 9, 2024
@leanprover-community-mathlib4-bot
Copy link
Collaborator

leanprover-community-mathlib4-bot commented Apr 9, 2024

Mathlib CI status (docs):

  • ❗ Std CI can not be attempted yet, as the nightly-testing-2024-04-09 tag does not exist there yet. We will retry when you push more commits. If you rebase your branch onto nightly-with-mathlib, Std CI should run now. (2024-04-09 08:46:12)
  • ❌ Mathlib branch lean-pr-testing-3851 built against this PR, but linting failed. (2024-04-10 12:19:10) View Log
  • ❌ Mathlib branch lean-pr-testing-3851 built against this PR, but linting failed. (2024-04-10 12:40:50) View Log

leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Apr 10, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Apr 10, 2024
@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added the breaks-mathlib This is not necessarily a blocker for merging: but there needs to be a plan label Apr 10, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Apr 10, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Apr 10, 2024
@david-christiansen david-christiansen added will-merge-soon …unless someone speaks up full-ci labels Apr 12, 2024
h₁ : start ≤ stop
h₂ : stop ≤ as.size
start_le_stop : start ≤ stop
stop_le_array_size : stop ≤ array.size
Copy link
Collaborator

@digama0 digama0 Apr 12, 2024

Choose a reason for hiding this comment

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

Suggested change
stop_le_array_size : stop ≤ array.size
stop_le_size : stop ≤ array.size

this still seems sufficiently precise/descriptive and is less annoyingly long

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ha, that's what I originally had! I changed it because Subarray.size exists with a different meaning.

I'll hold off on the merge for a short bit and think it over a second time.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, yeah that is awkward. I wonder if we could change the name to avoid confusion (i.e. Subarray.length)? I think we will want names for both of these quantities eventually (e.g. in lemmas), so it would be good to think about what names are clear and distinct. length and size? size and fullSize? extent and maxStop? Or just use t.array.size.

As a pedantic note, prefix naming convention would yield stop_le_size_array.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On reflection, I expect that this projection will not be used so often (mostly I expect users to be using the API here, or writing meaningful operators that preserve the invariants - borne out by the very small changes needed for Std and Aesop and the fact that Mathlib itself needed none), so I'll go with explicitness over concision.

Thanks!

@david-christiansen
Copy link
Contributor Author

OK, Mathlib builds against leanprover-community/batteries#733 and leanprover-community/aesop#122 locally for me.

@david-christiansen david-christiansen added this pull request to the merge queue Apr 13, 2024
Merged via the queue into leanprover:master with commit 864221d Apr 13, 2024
19 checks passed
@fgdorais
Copy link
Contributor

Closes #2164

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaks-mathlib This is not necessarily a blocker for merging: but there needs to be a plan toolchain-available A toolchain is available for this PR, at leanprover/lean4-pr-releases:pr-release-NNNN will-merge-soon …unless someone speaks up
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants