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

StubCAS is built with a builder #6582

Merged
merged 2 commits into from Oct 2, 2018

Conversation

Projects
None yet
3 participants
@illicitonion
Copy link
Contributor

illicitonion commented Oct 2, 2018

I'm about to add more variations, and the functions are getting awkward.

StubCAS is built with a builder
I'm about to add more variations, and the functions are getting awkward.
@blorente
Copy link
Contributor

blorente left a comment

Thanks! I've wanted to do this for a while! I had thought of the following constraints when thinking about this:

  • The order of operations on the builder should not matter
  • The overhead shouldn't be too high (assuming .bytes() does not create a copy, which I think is true)
@@ -2161,11 +2161,11 @@ mod tests {
/// Create a StubCas with a file and a directory inside.
///
pub fn new_cas(chunk_size_bytes: usize) -> StubCAS {
StubCAS::with_content(
chunk_size_bytes as i64,

This comment has been minimized.

@blorente

blorente Oct 2, 2018

Contributor

Is the change of this to usize related to the rest of the PR?
I'm guessing it's there because there's no reason for it to not be a usize, but I might be missing something.
Even if it's not directly related, I'm not against it being in this PR.

This comment has been minimized.

@illicitonion

illicitonion Oct 2, 2018

Contributor

It's related insofar as the reason this was an i64 before was so that we could use the magic value -1 to mean "always error". The reason we did that was because adding more constructor parameters was hassle, so magic values were easier. Now that new constructor parameters are cheap, we can be explicit (use a named bool) rather than implicit (use a magic value of another field) :)

@baroquebobcat
Copy link
Contributor

baroquebobcat left a comment

This is much clearer. Thanks!

StubCAS::with_unverified_content_and_port(chunk_size_bytes, blobs, 0)
pub fn port(mut self, port: u16) -> Self {
if self.port.is_some() {
panic!("Can't set chunk_size twice");

This comment has been minimized.

@baroquebobcat

baroquebobcat Oct 2, 2018

Contributor

Can't set port twice?

This comment has been minimized.

@illicitonion

illicitonion Oct 2, 2018

Contributor

Done, thanks!

@illicitonion illicitonion merged commit fd7c2ee into pantsbuild:master Oct 2, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@illicitonion illicitonion deleted the twitter:dwagnerhall/remoting/casbuilder branch Oct 2, 2018

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