Refactoring #168

KalitaAlexey · 2017-02-04T11:26:21Z

Hi everybody.

I made small refactoring.
Moved actions.rs to actions/mod.rs.
Extracted parsing compiler messages into actions/compiler_message_parsing.rs

nrc

Thanks for this PR! I think the idea of factoring out stuff from build in actions.rs is a good one, however, I think there a few places where we can improve on the design in this PR. I left a few comments inline as a first pass, but this is kind of a hard PR to review because of the diff, so we might have to iterate on it a bit...

nrc · 2017-02-06T19:00:26Z

src/actions/build/mod.rs

+
+    fn lock_previous_results(&self) -> MutexGuard<Results> {
+        self.previous_results.lock().expect("previous results lock failed")


I would inline this function - I think repeating the message string is OK and makes it a bit clearer what is going on

I am not quite sure whether you are agree with this change or not)

I agree with changing unwrap to expect, but not with factoring out a function.

nrc · 2017-02-06T19:05:50Z

src/actions/build/mod.rs

@@ -76,11 +71,10 @@ impl Build {
            }
        }

-        fn convert_build_results_to_notifications(build_results: &Results,
-                                                  project_path: &Path)
+        fn convert_results_to_notifications(results: &Results, project_path: &Path)


It's not really your fault, more a weakness in Git, but this diff is really hard to review. When you're doing refactoring, could you make your commits as small as possible please? E.g., here, I think factoring out process_build_result should be its own commit, deleting the structs one commit, and the other renaming another commit.

nrc · 2017-02-06T19:32:15Z

src/actions/build/compiler_message_parser.rs

@@ -0,0 +1,96 @@
+use std::path::PathBuf;


Again, not your fault, but for easier review, could you separate the moving of code to another file into a separate commit from any changes?

These new files should have the copyright boilerplate

I'm a bit confused about the module structure here - we have build.rs at the root, plus a complex build submodule of actions. That seems confusing. I assume that actions/build is just the stuff from the build action. I think it would be better just to have this module inside actions, and not have a build submodule of actions.

@nrc,
I moved it in another file because I like small files)

You can keep it another file, just not in a new module/directory, i.e., add actions/compiler_message_parser.rs

nrc · 2017-02-06T19:47:09Z

src/actions/build/mod.rs

+
+type Results = HashMap<PathBuf, Vec<Diagnostic>>;
+
+pub struct Build {


I'm not sure if this is just a naming thing or whether this abstraction could be improved, but it seems kind of wrong at the moment. A Build is not a single build, it seems to be more like a BuildManager or BuildHandler, but that has some overlap with the BuildQueue. I think I'm not convinced that there is something worth abstracting out of ActionHandler here.

To be clear, I think you are correct about pulling out some of the build-related functions from inside build, but I don't think pulling out the data into a Build struct is the right approach to doing this.

nrc · 2017-02-06T19:49:47Z

src/actions/build/mod.rs

+        results.clear();
+    }
+
+    pub fn execute(&self, project_path: &Path, priority: BuildPriority, out: &Output) {


Similar to the last comment, this is method is just handling the build request and so I think it should be in the action handler. It is not executing the build, it is enqueing it, and then handling the result.

nrc · 2017-02-06T19:51:33Z

src/actions/build/compiler_message_parser.rs

+    }
+}
+
+pub struct CompilerMessageParser {}


If there is no data, this struct should not exist.

nrc · 2017-02-06T19:51:46Z

src/actions/build/compiler_message_parser.rs

+pub struct CompilerMessageParser {}
+
+impl CompilerMessageParser {
+    pub fn parse(message: &str) -> Result<FileDiagnostic, ParseError> {


This could be a module-level function

@nrc,
Yes it could.
I unlearned to use functions.

KalitaAlexey · 2017-02-06T20:10:57Z

@nrc,
Should I close this PR or just force push to separate things by commits?

nrc · 2017-02-06T20:45:22Z

force push would be good, thanks

KalitaAlexey · 2017-02-06T20:46:18Z

@nrc,
I will do that tomorrow.

KalitaAlexey · 2017-02-07T17:50:39Z

@nrc,
Now it is indisputable diff.

nrc

Looks good, just needs the copyright notice on the new file.

nrc · 2017-02-07T22:54:49Z

src/actions/compiler_message_parsing.rs

@@ -0,0 +1,92 @@
+use std::path::PathBuf;


Needs copyright notice

nrc · 2017-02-09T04:41:41Z

Thank you!

KalitaAlexey mentioned this pull request Feb 5, 2017

dead_code doesn't show unused struct as unused rust-lang/rust#39558

Closed

nrc suggested changes Feb 6, 2017

View reviewed changes

KalitaAlexey added 2 commits February 7, 2017 20:38

Moved actions.rs to actions/mod.rs

ee136ef

Extracted parsing compiler messages.

cf6f53a

KalitaAlexey changed the title ~~Refactored fn build out of struct ActionHandler.~~ Refactoring Feb 7, 2017

nrc approved these changes Feb 9, 2017

View reviewed changes

Added the copyright notice.

aabeabc

nrc merged commit c1fa6fe into rust-lang:master Feb 9, 2017

KalitaAlexey deleted the refactoring branch February 9, 2017 06:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactoring #168

Refactoring #168

KalitaAlexey commented Feb 4, 2017 •

edited

Loading

nrc left a comment

nrc Feb 6, 2017

KalitaAlexey Feb 6, 2017

nrc Feb 6, 2017

nrc Feb 6, 2017

nrc Feb 6, 2017

nrc Feb 6, 2017

nrc Feb 6, 2017

KalitaAlexey Feb 6, 2017

nrc Feb 6, 2017

nrc Feb 6, 2017

nrc Feb 6, 2017

nrc Feb 6, 2017

nrc Feb 6, 2017

nrc Feb 6, 2017

KalitaAlexey Feb 6, 2017

KalitaAlexey commented Feb 6, 2017 •

edited

Loading

nrc commented Feb 6, 2017

KalitaAlexey commented Feb 6, 2017

KalitaAlexey commented Feb 7, 2017

nrc left a comment

nrc Feb 7, 2017

KalitaAlexey Feb 9, 2017

nrc commented Feb 9, 2017


		fn lock_previous_results(&self) -> MutexGuard<Results> {
		self.previous_results.lock().expect("previous results lock failed")


		type Results = HashMap<PathBuf, Vec<Diagnostic>>;

		pub struct Build {

Refactoring #168

Refactoring #168

Conversation

KalitaAlexey commented Feb 4, 2017 • edited Loading

nrc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KalitaAlexey commented Feb 6, 2017 • edited Loading

nrc commented Feb 6, 2017

KalitaAlexey commented Feb 6, 2017

KalitaAlexey commented Feb 7, 2017

nrc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nrc commented Feb 9, 2017

KalitaAlexey commented Feb 4, 2017 •

edited

Loading

KalitaAlexey commented Feb 6, 2017 •

edited

Loading