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

Reword &str coercion into viewing #18762

Merged
merged 1 commit into from
Nov 9, 2014
Merged

Conversation

mdinger
Copy link
Contributor

@mdinger mdinger commented Nov 8, 2014

I had slight confusion when using this as a reference and was told it was imprecise. Most of the rewording was suggested by @huonw.

cc @steveklabnik

@aturon
Copy link
Member

aturon commented Nov 8, 2014

Thanks for the PR! This looks like an improvement to me, but I want to give @steveklabnik a chance to weigh in.

@mdinger
Copy link
Contributor Author

mdinger commented Nov 8, 2014

Specifically, with coerce as before, I interpreted it to mean that the underlying String was modified by as_slice() which was puzzling.

@steveklabnik
Copy link
Member

We haven't made a formal decision to use this 'view' terminology, but I do like it. I am neutral to mildly positive about this, so if @aturon likes it, r=me

@mdinger
Copy link
Contributor Author

mdinger commented Nov 8, 2014

I don't get how r=me is supposed to work. I could just r=me at aturon or steveklabnik or alexcrichton and bors would automatically merge without their explicit approval (I know how it's supposed to be written but am not trying to write it properly here)? Isn't that just bypassing the review process?

@aturon
Copy link
Member

aturon commented Nov 9, 2014

@Mdiner It's just an informal way of telling other reviewers that you're personally happy with the PR. Someone with reviewing rights still has to sign off.

(bors will only pay attention to r+ and r=somereviewer comments on the commit from people with reviewing rights.)

@aturon
Copy link
Member

aturon commented Nov 9, 2014

@steveklabnik Yeah, we should think more broadly about this terminology -- might be worth raising at the next weekly meeting. Want me to add to the agenda?

In any case, I'm happy to land this for now.

@mdinger
Copy link
Contributor Author

mdinger commented Nov 9, 2014

Okay. Thanks for clarification

@steveklabnik
Copy link
Member

@aturon sounds great, yeah. I'll come prepared :)

bors added a commit that referenced this pull request Nov 9, 2014
I had slight confusion when using this as a reference and was told it was imprecise. Most of the rewording was suggested by @huonw.

cc @steveklabnik
@bors bors closed this Nov 9, 2014
@bors bors merged commit c39e9f3 into rust-lang:master Nov 9, 2014
@mdinger mdinger deleted the str_coerce branch January 2, 2015 20:51
lnicola pushed a commit to lnicola/rust that referenced this pull request Jan 7, 2025
…salsa-cancellation-error

internal: wrap `salsa::Cycle`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants