ArrayMethod and StringMethod implementation should be refactored into a generic method structure
feat: ArrayMethod and StringMethod are two structures with identical members (StringMethod members are all public).
These two are doing largely the same work (supplying methods to one of the objects, string or array) but the implementation differs completely how the two structures does this. It feels like both of the files were written by two different persons at two different times.
ArrayMethod's methods largely defines each in-shell method in a separate source code method. StringMethod only has one handle method and uses builtin macros, within the scope of the method. All logic are decided in a huge match block.
The logic behind Array and String methods has to work with the name of the variable directly and resolve it's value manually. Ideally you want to write functions that takes typed arguments which works with values directly and does not have to de reference variable names or initiate array expressions. That should be the job of a parser.
The method structures are kind of parsed, but it could be better. If we look at ArrayMethod structure we will see a couple of problems. (StringMethod structure is the same)
pub struct ArrayMethod<'a> {
method: &'a str,
variable: &'a str,
pattern: Pattern<'a>,
selection: Option<&'a str>,
}
- The ArrayMethod structure has it's method name as one of it's argument. This serves no purpose in the code which is the method, other than to call it. The ArrayMethod should be redesigned so to either not need to know the name of the method (the parser calls the method as soon as it has read the method name), or ArrayMethod is a composit structure which consists of a structure which has a method name member and an argument's member which is a structure containing the arguments strictly needed to perform the method operation.
- The ArrayMethod has one variable argument. Should it not be an array of variables? We can make more interesting methods if we were working with more than one argument.
- The ArrayMethod has a pattern member. The pattern member is an enum which looks like this:
pub enum Pattern<'a> {
StringPattern(&'a str),
Whitespace,
}
I've tested what's put inside it and it seams like anything after the first argument in a in-shell method is put inside StringPattern as raw text. (Correct me if I'm wrong). Here is the rest of the variables I want my method to work with, in raw script source text. Now I have to manually parse them. 4. The ArrayMethod has a selection member. I don't know what that is, but I have a feeling that it should be pre-parsed away.
I suggest we go back to the drawing board with how method parsing should be done as it is a bit of a mess of how it's implemented.
My suggestion of a generic method structure is just to have an array of preparsed variables. No variable names needed to be de-referenced. No array expression needing to be created. The array should be typed and its contents must match the intended methods signature to be able to run.
Example:
pub enum Typed<'a> {
Array(&'a [str]),
String(&'a str),
}
pub struct MethodStruct<'a> {
variables: &'a [Typed]
}
The three culprit files are hidden in src/lib/expansion/methods/
BREAKING CHANGE: Changes in the datastructure, internal
performance: Might improve
usability: Adding methods for either ArrayMethod or StringMethod should be easier.
maintainability: Should remove code, making it easier to maintain.
reason: Refactoring for refactorings sake.
PS.
I've been rambling on about ArrayMember and StringMember. I just think there can be a lot of improvements to the method parsing in the files mentioned and that something should be done before even more methods needs to be written.
Oh and also testing tools should be rewritten. Why not test with the real ion shell instead of manually inserting variables into the shell heap with the DummyExpander?