Analyzer changes

[neunenak]

This is a refactoring commit to the analyzer code that hopefully makes it easier to understand what the analyzer is doing.

Analyzer::analyze() now instantiates an Analyzer, and all of the data structures containing items have been moved to being members of thatAnalyzer struct, rather than some being on the struct and accessed through self and others being bare variables in the function scope.

The definitions HashMap is moved to being a field on the Analyzer struct, rather than using a closure defined in function scope.

analyze_set is moved to being an associated function so it works the same way as the other analyze_* functions.

[casey]

This seems reasonable!

I changed 'local to 'run, just cause that's used elsewhere for a second, shorter lifetime for runtime data.

Can it be refactored so that the main function takes self? I'd prefer that a little over having a local variable Analyzer in scope named analyzer.

Can definitions be a variable on Analyzer, so define() can be a method and we can avoid an additional newtype?

[neunenak]

Can it be refactored so that the main function takes self? I'd prefer that a little over having a local variable Analyzer in scope named analyzer.

I think this is possible, but that would require going back to the pattern where there's a (public) Analyzer::analyze associated function, that does nothing except creating an instance of Analyzer and then calling an analyzer.justfile(...) method on it. And that would involve having one associated function and one method on Analyzer with basically the same (and rather lengthy) parameter list. So in this case I think it's worthwhile to instantiate a local Analyzer within Analyzer::analyze and avoid the repetition. Maybe name it something like let mut tables = Analyzer::default()?

[casey]

I think I prefer having a function and a method, so that we can use self. I don't really mind the parameter duplication, and it's nice to get the syntax highlighting on the self and have it be a method.

[casey]

Is this ready to look at again? GitHub notifies on pushes, but I don't know if a push means "ready to review".

[neunenak]

Is this ready to look at again? GitHub notifies on pushes, but I don't know if a push means "ready to review".

Ah yeah, it is, I think I've addressed everything.

[casey]

Overall I feel like these changes are kind of a wash. I like moving define into a method, and from_table is certainly better than from_setting_iter, but it's unclear to me that the rest of the changes are that beneficial for clarity, when there are downsides like needing a recipe_names variable to avoid borrowing self, and the declaration of an additional assignments variable.

[neunenak]

Overall I feel like these changes are kind of a wash. I like moving define into a method, and from_table is certainly better than from_setting_iter, but it's unclear to me that the rest of the changes are that beneficial for clarity, when there are downsides like needing a recipe_names variable to avoid borrowing self, and the declaration of an additional assignments variable.

Feel free to keep whatever code you think is worth merging out of this PR and get rid of the rest. The main thing that was bugging me was that the initial iteration over the AST items was modifying some variables in local scope and some variables on self with no particular rhyme or reason to them.

[casey]

Merged! I did a little fiddling and got rid of one of the temporaries.