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

Remove Params::expect_single compatibility API #6766

Merged
merged 9 commits into from Nov 27, 2018

Conversation

Projects
None yet
2 participants
@stuhood
Copy link
Member

stuhood commented Nov 14, 2018

Problem

Params::expect_single was a compatibility API that was meant to simplify the transition from "Node has a single Subject" to "Node has a set of Params", but usage of that API inherently blocks exposing the ability to pass multiple Params in a run (and thus blocks #6478).

Solution

Remove the three distinct usages of Params::expect_single by:

  1. simplifying the signature of scheduler_execute, which was returning a copy of the subject that was unused anyway
  2. directly displaying Params rather than looking for a single subject value
  3. inlining Select::gen_node and resolving a TODO to consume Entry::Param and Params::find to expect parameter values

Result

Progress toward #6478, and a ~5% speedup due to taking better advantage of the static rule_graph in Select.

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Nov 14, 2018

Commits are useful to review independently.

@illicitonion
Copy link
Contributor

illicitonion left a comment

Looks great!

I remain a little sad at Rust that there isn't something between a type alias and a newtype, so that ParamTypes could transparently act exactly like a BTreeSet, but have custom trait implementations (like Display/Debug)...

},
Err(f) => {
let val = match f {
Failure::Invalidated => create_exception("Exhausted retries due to changed files."),

This comment has been minimized.

@illicitonion

illicitonion Nov 14, 2018

Contributor

This is a very specific error message for a pretty generic function...

Show resolved Hide resolved src/rust/engine/src/core.rs Outdated

@stuhood stuhood force-pushed the twitter:stuhood/remove-expect-single branch from a991368 to 4c85c7a Nov 21, 2018

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Nov 27, 2018

Going to save some trees and assume that my clippy fix is enough to go green. Merging.

@stuhood stuhood merged commit 03523a0 into pantsbuild:master Nov 27, 2018

1 check failed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details

@stuhood stuhood deleted the twitter:stuhood/remove-expect-single branch Nov 27, 2018

@stuhood stuhood referenced this pull request Nov 27, 2018

Closed

Add API for root requests with multiple parameters #6478

4 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment