Added function formatting

[CpazR]

Added a number of keywords and altered some pre-existing functionality for GMS 2.3

• function
• constructor
• new
• delete

Some of this applies to structs as well. See added integration tests for examples of formatting.

NOTE: This is my first real stab as using rust. I ask to keep that in mind when reviewing code. This is very much a learning experience.

[sanbox-irl]

Oh wow -- my plan had been a complete rewrite of this for 2.3. will take a look tomorrow! Appreciate the effort!

[sanbox-irl]

Quite funny btw that you say that this is your first stab at Rust since this project was also my first stab at Rust!

[sanbox-irl]

Okay! I have taken a look at it, and I first want to say how much I appreicate the PR and how nice the Rust looks! Overall, the whole repo needs more documentation, but I'm glad you managed to parse through how it works. That's really great for me to see.

So, tldr: I think we need a bit more before we merge in this pull request, and I hope these aren't disheartening to hear.

The first is this silly, but not pathological, case:

#[test]
fn local_variable_function() {
    let input = "var x = function(input){show_debug_message(input)}";
    let format = "var x = function(input) { show_debug_message(input) }";

    assert_eq!(run_test(input), format);
}

Right now, we can't pass that test (in fact, we quite garble the code). The pathological case which we can't support yet, but probably need to is the following:

var test1 = function(input) { show_debug_message(input + " One"); }, test2 = function(input) { show_debug_message(input + "Two"); }

test1("hello");
test2("hello");

Now, I don't actually think this is a hard solution. Right now, you've used function as a statement, but what if we move function to be a kind of Expression? Then we'll be able to handle functions in these locations without issue, and we can also seriously simplify out assignments. We can also make new an expression too, so we don't need to mark anything else up further.

Roughly sketching it out, new would look about like this this:

#[derive(Debug)]
pub enum Expr<'a> {
    // -- snip!
    // *new(implicit)* *comments_before_call* *call*
    New {
        comments_before_call: ExprBox<'a>,
        // will always contain a `Call`, but gives us flexibility to format more incorrect, but coherent
        // code, such as "new 5";
        call: ExprBox<'a>,
    },
    // -- snip!
}
    

And then we can separate out Call and some new Expression Function, which will let us slot in the Function into array lists. This also means that Calls won't need to explicitly mark if they're a constructor or not, since no function call can really be a constructor, but a function declaration can.

Separately, I'm hesitant to merge these into main without also supporting 2.3 era syntax such as Struct Literals (which personally, I find myself using quite a bit at work projects), but that doesn't need to be your responsibility. Maybe I can make a branch for 2.3, which will give me a chance to clean up a lot of my code, and then we can merge this in to there once the above has been handled in some way.

What do you think of all of this? Apologies for the wall of text!

[CpazR]

The first is this silly, but not pathological, case:

#[test]
fn local_variable_function() {
    let input = "var x = function(input){show_debug_message(input)}";
    let format = "var x = function(input) { show_debug_message(input) }";

    assert_eq!(run_test(input), format);
}

Right now, we can't pass that test (in fact, we quite garble the code). The pathological case which we can't support yet, but probably need to is the following:

var test1 = function(input) { show_debug_message(input + " One"); }, test2 = function(input) { show_debug_message(input + "Two"); }

test1("hello");
test2("hello");

Ah. Yeah. That being left in was an oversight on my part. I think I have a fix for this though. My issue was it was hacky (much like the lambda implementation). It'll likely get redone with the other recommended changes.

Now, I don't actually think this is a hard solution. Right now, you've used function as a statement, but what if we move function to be a kind of Expression? Then we'll be able to handle functions in these locations without issue, and we can also seriously simplify out assignments. We can also make new an expression too, so we don't need to mark anything else up further. ... And then we can separate out Call and some new Expression Function, which will let us slot in the Function into array lists. This also means that Calls won't need to explicitly mark if they're a constructor or not, since no function call can really be a constructor, but a function declaration can.

Agree with this 100%. This approach makes much more sense in hindsight, now having figured out how everything works better.

Separately, I'm hesitant to merge these into main without also supporting 2.3 era syntax such as Struct Literals (which personally, I find myself using quite a bit at work projects), but that doesn't need to be your responsibility. Maybe I can make a branch for 2.3, which will give me a chance to clean up a lot of my code, and then we can merge this in to there once the above has been handled in some way.

That's fair. The initial goal of this branch was to fix function formatting exclusively, which ended up including constructors for structs. However, I can see the implementation of struct literals being fairly involved, so I think I'd make more sense as it's own branch.

If you did want to make a branch for 2.3 additions I could change the target branch to that. Of course, if you're rewriting everything anyway, I'd also not have too much of an issue tossing this branch, depending on how far you are.

In the meantime, I'll try adding those changes when I get the chance. Which will likely take a while

[sanbox-irl]

Will make a new branch tonight and push in the little fiddly changes I have. I have nothing that would upset what you have -- mostly I'm trying to reduce the complexity of lifetimes as I now have a significantly better idea of how to program elegantly in Rust. This code base is pretty brute force with the lifetimes and what not

[CpazR]

I've managed to simplify everything into two expressions. Many of the additional properties to statements have been removed.

Some of the modifications for statement parsing and printing (calls specifically) I've kept in simply because I'm not sure of a better way of handling lambdas.

I'll be trying to implement that edge case with a variable series of functions. Not sure how often that'd be used, but I could probably manage it.

[mathew-odwyer]

Has this PR gone stale or is it still in the works?

[CpazR]

Has this PR gone stale or is it still in the works?

There were talks of rewriting this tool, if memory serves. But this branch isn't being worked on by me anymore. Probably safe to close this PR since there are still outstanding issues with this implementation.

Also GM has changed substantially since and this would need even further reworking to be kept up to date.

Ideally, the whole tool is re-written to be more modular and manageable with the rapid additions as of late.

[mathew-odwyer]

@sanbox-irl if this is something that will be rewritten, I would love to be a large part of the development. If there is something that GM is missing is this. Coming from a C# background and using CodeMaid it's something I would love.