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

Use .find().map() instead of .filter_map().next() #9436

Merged
merged 2 commits into from Jan 28, 2016
Merged

Conversation

@fstr
Copy link

fstr commented Jan 26, 2016

Patch for issue #9403

Review on Reviewable

@highfive
Copy link

highfive commented Jan 26, 2016

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!
@KiChjang KiChjang self-assigned this Jan 26, 2016
None
}
}).next()
list.iter().find(|&&(ref k, _)| k == &name.0).map(|&(_, ref v)| USVString(v.clone()))

This comment has been minimized.

Copy link
@KiChjang

KiChjang Jan 26, 2016

Member

Could this have one less & in the .find() method?

This comment has been minimized.

Copy link
@KiChjang

KiChjang Jan 26, 2016

Member

Actually, is it possible to not have |&(ref k, _)| at all and simply have |ref kv| kv.0 == name.0 instead?

This comment has been minimized.

Copy link
@nox

nox Jan 27, 2016

Member

Or just |&kv| kv.0 == name.0 and |&kv| USVString(kv.1.clone()).

@frewsxcv
Copy link
Member

frewsxcv commented Jan 27, 2016

In its current state, I find the previous code much more readable than with these changes

@frewsxcv
Copy link
Member

frewsxcv commented Jan 27, 2016

...though I think that can be improved if it was broken up into one line per each method call

@KiChjang
Copy link
Member

KiChjang commented Jan 27, 2016

Agreed, let's break it down into two lines.

Florian Strübe
@fstr
Copy link
Author

fstr commented Jan 27, 2016

I tried to implement all the suggestions.

It's also possible to not work with & and ref. Is there any disadvantage?

list.iter().find(|kv| kv.0 == name.0)
    .map(|kv| USVString(kv.1.clone()))
@KiChjang

This comment has been minimized.

|ref kv| should also be |&kv| here as well.

@KiChjang
Copy link
Member

KiChjang commented Jan 27, 2016

r=me post-nit, it's definitely fine to just use &kv.

@bors-servo delegate+

@bors-servo
Copy link
Contributor

bors-servo commented Jan 27, 2016

✌️ @fstr can now approve this pull request

@fstr
Copy link
Author

fstr commented Jan 28, 2016

find(|&kv| ...).map(|&kv| ...)
/Users/florian/servo/components/script/dom/urlsearchparams.rs:82:19: 82:22 error: cannot move out of borrowed content [E0507]
/Users/florian/servo/components/script/dom/urlsearchparams.rs:82             .map(|&kv| USVString(kv.1.clone()))
                                                                                   ^~~
/Users/florian/servo/components/script/dom/urlsearchparams.rs:82:20: 82:22 note: attempting to move value to here
/Users/florian/servo/components/script/dom/urlsearchparams.rs:82             .map(|&kv| USVString(kv.1.clone()))
                                                                                    ^~
/Users/florian/servo/components/script/dom/urlsearchparams.rs:82:20: 82:22 help: to prevent the move, use `ref kv` or `ref mut kv` to capture value by reference

So I leave it as it is.

@KiChjang
Copy link
Member

KiChjang commented Jan 28, 2016

@bors-servo r+

Thanks!

@bors-servo
Copy link
Contributor

bors-servo commented Jan 28, 2016

📌 Commit e65d725 has been approved by KiChjang

@bors-servo
Copy link
Contributor

bors-servo commented Jan 28, 2016

Testing commit e65d725 with merge 562fe03...

bors-servo added a commit that referenced this pull request Jan 28, 2016
Use .find().map() instead of .filter_map().next()

Patch for issue #9403

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.svg" height="40" alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9436)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jan 28, 2016

💔 Test failed - mac-dev-unit

@jdm
Copy link
Member

jdm commented Jan 28, 2016

@bors-servo: retry

  • buildbot infrastructure problem?
@bors-servo
Copy link
Contributor

bors-servo commented Jan 28, 2016

Previous build results for android, gonk, linux-dev, mac-rel-css, mac-rel-wpt are reusable. Rebuilding only linux-rel, mac-dev-unit...

@bors-servo
Copy link
Contributor

bors-servo commented Jan 28, 2016

💔 Test failed - linux-rel

@nox
Copy link
Member

nox commented Jan 28, 2016

@nox nox added S-awaiting-merge and removed S-tests-failed labels Jan 28, 2016
@bors-servo
Copy link
Contributor

bors-servo commented Jan 28, 2016

Previous build results for android, gonk, linux-dev, mac-dev-unit, mac-rel-css, mac-rel-wpt are reusable. Rebuilding only linux-rel...

@bors-servo
Copy link
Contributor

bors-servo commented Jan 28, 2016

@bors-servo bors-servo merged commit e65d725 into servo:master Jan 28, 2016
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.