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

Add peek_index to Enumerate #130682

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

Add peek_index to Enumerate #130682

wants to merge 7 commits into from

Conversation

x4exr
Copy link
Contributor

@x4exr x4exr commented Sep 22, 2024

@rustbot
Copy link
Collaborator

rustbot commented Sep 22, 2024

r? @cuviper

rustbot has assigned @cuviper.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Sep 22, 2024
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@cuviper
Copy link
Member

cuviper commented Sep 23, 2024

While I do like the name peek_index, it's not what the libs-api team agreed on. This should be implemented as next_index like they preferred, and then peek_index or other names can be discussed in the tracking issue.

@cuviper
Copy link
Member

cuviper commented Sep 24, 2024

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 24, 2024
@x4exr
Copy link
Contributor Author

x4exr commented Sep 24, 2024

While I do like the name peek_index, it's not what the libs-api team agreed on. This should be implemented as next_index like they preferred, and then peek_index or other names can be discussed in the tracking issue.

My apologies, once I return today I will update the PR.

@tgross35
Copy link
Contributor

Just an fyi, this should get a squash before merge

@alex-semenyuk
Copy link
Member

@x4exr
From wg-triage. Do you have any updates on this PR?

@x4exr
Copy link
Contributor Author

x4exr commented Nov 9, 2024

Iirc i was supposed to change the feature name before this is ready to merge

@alex-semenyuk
Copy link
Member

@x4exr
Thanks for your contribution.

Iirc i was supposed to change the feature name before this is ready to merge

Could you please do this and

Just an fyi, this should get a squash before merge

@x4exr
Copy link
Contributor Author

x4exr commented Jan 20, 2025

if I'm being honest I have no clue what I'm doing with git
I once tried squashing this and it vasicly did nothing

@tgross35
Copy link
Contributor

It should be something like git rebase -i master --keep-base (--keep-base means it won't actually change the base, i.e. rebases on to whatever master looked like when you created the branch rather than what master looks like today). This will open an editor with a bunch of lines starting with pick ... (one line per commit). For everything except the first commit, change pick to f or fixup, then save & quit.

This will combine all later commits into the first one, discarding their messages (s or squash instead of f would combine their messages, but it doesn't look like there is anything to keep here). Then once that is done, check that git log/git show HEAD looks correct and git push --force-with-lease to update the PR.

@Dylan-DPC
Copy link
Member

@x4exr any updates on this? thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants