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

doc: add docstrings and examples for String functions #4332

Merged
merged 4 commits into from
Jun 5, 2024

Conversation

austinletson
Copy link
Contributor

Add docstrings, usage examples, and doctests for String.get', String.next', String.posOf, String.revPosOf.

@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 Jun 3, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Jun 3, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Jun 3, 2024
@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added breaks-mathlib This is not necessarily a blocker for merging: but there needs to be a plan release-ci Enable all CI checks for a PR, like is done for releases labels Jun 3, 2024
@leanprover-community-mathlib4-bot
Copy link
Collaborator

leanprover-community-mathlib4-bot commented Jun 3, 2024

Mathlib CI status (docs):

  • ❌ Mathlib branch lean-pr-testing-4332 built against this PR, but testing failed. (2024-06-03 22:54:13) View Log
  • ❗ Batteries/Mathlib CI will not be attempted unless your PR branches off the nightly-with-mathlib branch. Try git rebase f65e3ae9859a4288b94b4a9fda93f406244d8dbc --onto 81f5b07215b077f9b5be677f8689617512d3322c. (2024-06-04 00:14:38)

Add docstrings, usage examples, and doctests for `String.get'`, `String.next'`,
`String.posOf`, `String.revPosOf`.
Comment on lines 266 to 268
Given `def abc := "abc"` and `def lean := "L∃∀N"`,
* `abc.get' 0 (by decide) = 'a'`
* `lean.get' (0 |> lean.next |> lean.next) (by decide) = '∀'`
Copy link
Collaborator

Choose a reason for hiding this comment

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

May clearer without the defs, and just inlining the strings?

Copy link
Collaborator

Choose a reason for hiding this comment

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

and below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the single "abc" case. I agree this is more apparent.

I have streamlined the repetitive examples by replacing the "Given, def..." line with a let statement in the bullet:

`let lean := "L∃∀N"; lean.get' (0 |> lean.next |> lean.next) (by decide) = '∀'`

It seems off to me to inline the string in the repetitive cases:

`"L∃∀N".get' (0 |> "L∃∀N".next |> "L∃∀N".next) (by decide) = '∀'`.

However, please let me know if you still prefer the inlining in the repetitive cases, and I will update the examples accordingly.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks good, thanks.

@semorrison
Copy link
Collaborator

Otherwise, looks good.

@semorrison semorrison added awaiting-author Waiting for PR author to address issues and removed breaks-mathlib This is not necessarily a blocker for merging: but there needs to be a plan labels Jun 4, 2024
@austinletson
Copy link
Contributor Author

awaiting-review

@github-actions github-actions bot added awaiting-review Waiting for someone to review the PR and removed awaiting-author Waiting for PR author to address issues labels Jun 4, 2024
@semorrison semorrison removed the awaiting-review Waiting for someone to review the PR label Jun 5, 2024
@semorrison semorrison added this pull request to the merge queue Jun 5, 2024
Merged via the queue into leanprover:master with commit 644c1d4 Jun 5, 2024
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-ci Enable all CI checks for a PR, like is done for releases toolchain-available A toolchain is available for this PR, at leanprover/lean4-pr-releases:pr-release-NNNN
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants