Added wait_to_build time to config #391

robbym · 2017-07-04T04:13:55Z

Issue #364

nrc

This looks great, thank you! There are two very minor improvements I've noted inline.

nrc · 2017-07-04T05:33:03Z

src/build.rs

+                config.wait_to_build
+            };
+
+            thread::sleep(Duration::from_millis(wait_to_build as u64));


You could make the config option u64 and then you wouldn't need this cast.

Sure. I need to add a new impl for ConfigType for that.
Should the variant name be "<unsigned 64bit integer>"?

that sounds good

nrc · 2017-07-04T05:33:36Z

src/config.rs

@@ -157,6 +157,7 @@ create_config! {
    build_bin: String, String::new(), false, "cargo check --bin <name>";
    cfg_test: bool, true, false, "build cfg(test) code";
    unstable_features: bool, false, false, "enable unstable features";
+    wait_to_build: usize, 500, false, "time between receiving a change notification and starting build";


Could you add "in milliseconds" to the description please?

nrc · 2017-07-04T05:34:12Z

src/build.rs

@@ -166,7 +163,12 @@ impl BuildQueue {
        let (tx, rx) = channel();
        self.pending.lock().unwrap().push(tx);
        if priority == BuildPriority::Normal {
-            thread::sleep(Duration::from_millis(WAIT_TO_BUILD));
+            let wait_to_build = { // Release lock before we sleep


Good catch!

nrc · 2017-07-04T06:20:12Z

Thanks for the PR! Do you know what you would like to work on next? I can suggest something if you'd like.

robbym · 2017-07-04T06:21:38Z

No idea. Please do.

nrc · 2017-07-04T07:05:35Z

I have a couple of suggestions, do either sound interesting? Let me know if you have any questions.

dedup test_data #365 might be interesting and is certainly useful: we currently have to have one cargo project per test. We need to be able to execute tests in parallel, but if we do that and two tests have the same target directory, we'll get races on that directory. I think this should be fixable by specifying the output directory to Cargo when we run a test. I'm not sure how that works. I'm also not sure if that will fix this issue (but I think it should). I'm also not sure if this is something that can be limited to the test infrastructure, or has to be threaded through the RLS.
dead_code and unused* lint are annoying #252 is extremely annoying at the moment. A good minimal solution would be to add a config option for 'show_warnings' When that is false, the RLS does not report warnings to the user, only errors. That should be fairly easy; it would require exploring the error handling parts of the RLS. A nice extension would be to only show warnings when a file is saved (i.e., when the file is being edited, suppress warnings, and when it is saved, show them). That would be a bit harder because you'd have to coordinate the state of a file (I think this is tracked in the VFS, but you should check) with the error handling.

robbym · 2017-07-04T07:25:43Z

I think #252 would expose me to some more interesting bits of code. I'll try that first.

Added wait_to_build time to config

3294017

nrc reviewed Jul 4, 2017

View reviewed changes

Added u64 impl for ConfigType to prevent cast

b5e13e3

nrc merged commit a941737 into rust-lang:master Jul 4, 2017

nrc mentioned this pull request Jul 4, 2017

Make WAIT_TO_BUILD configurable #364

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added wait_to_build time to config #391

Added wait_to_build time to config #391

robbym commented Jul 4, 2017

nrc left a comment

nrc Jul 4, 2017

robbym Jul 4, 2017

nrc Jul 4, 2017

nrc Jul 4, 2017

nrc Jul 4, 2017

nrc commented Jul 4, 2017

robbym commented Jul 4, 2017

nrc commented Jul 4, 2017

robbym commented Jul 4, 2017

Added wait_to_build time to config #391

Added wait_to_build time to config #391

Conversation

robbym commented Jul 4, 2017

nrc left a comment

Choose a reason for hiding this comment

nrc Jul 4, 2017

Choose a reason for hiding this comment

robbym Jul 4, 2017

Choose a reason for hiding this comment

nrc Jul 4, 2017

Choose a reason for hiding this comment

nrc Jul 4, 2017

Choose a reason for hiding this comment

nrc Jul 4, 2017

Choose a reason for hiding this comment

nrc commented Jul 4, 2017

robbym commented Jul 4, 2017

nrc commented Jul 4, 2017

robbym commented Jul 4, 2017