Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tarmac-Styled Code Generation Support #33

Merged
merged 15 commits into from
Apr 26, 2024

Conversation

bibi-reden
Copy link
Contributor

@bibi-reden bibi-reden commented Apr 25, 2024

Hello!

This PR is meant to add in the feature requested in #23 related to tarmac code style. I've implemented a solution for this. Utilizing your tests, the emits are as followed so far:

Examples

Luau Emit (4 spaces)

return {
    bar = {
        ["baz.png"] = "rbxassetid://2",
    },
    ["foo.png"] = "rbxassetid://1",
}

Typescript Emit (4 spaces)

declare const assets: {
    bar: {
        "baz.png": "rbxassetid://2",
    },
    "foo.png": "rbxassetid://1",
}
export = assets

This also adds an extra configuration that is tarmac, which will be false by default.

Please feel free to request any changes or provide any edits or questions, and thank you for providing this useful tool!

- Implemented two modules.
- `ast`: Inspired from tarmac's implementation, extended for typescript support. Meant to serialize the provided lockfile's data and uses path components to achieve this in a way similar to tarmac.
- `tarmac`, related to the specific functions for codegen related to the style. This is interchangable respectively with both the generate_lua/ts functions.

- Added an extra option in the configuration to support tarmac style.

Thank you for making this tool!
Applies indent by a `number_of_spaces` rather than a tab. This allows for more customization (if wanted) in the `asphalt.toml` if a user wishes to configure their indentation size.
@jackTabsCode
Copy link
Owner

jackTabsCode commented Apr 25, 2024

Hey, thanks for your work! I haven't had time to look through the full implementation yet, but I am a bit concerned about the organization of code here before that.

Ideally, codegen.rs would contain all codegen related code. So main.rs should not really worry about what format the code is in. Ideally the main function would send off the lockfile to codegen, codegen would path it based on the desired code generation style, and receive back the string needed to write the file, which the main function would do.

So perhaps the structure could look somewhat more like:

main
  codegen/
    mod.rs
    flat.rs
    nested/
      mod.rs
      ast.rs
      ...

for example

Another minor thing-I am not a huge fan of naming the configuration (or the style itself) "tarmac" mode. I would much prefer if it the config option was named "style" and perhaps the options could be "flat" or "nested" (tarmac). We could then say in the readme, if you like the tarmac style, go with nested.

I'm also going to end up putting codegen options, since we have three at this point, in it's own section in the configuration. But this probably belongs in a different PR.

src/ast.rs Outdated Show resolved Hide resolved
@bibi-reden
Copy link
Contributor Author

Thanks for your feedback! I got a better understanding of what you would intend for your project, and I will be making the requested changes to the best of my ability.

On the tarmac naming, nested is perfectly fine, and providing a style option in the config is more extendable, I agree with you.
I will make those changes momentarily.

@bibi-reden
Copy link
Contributor Author

To add to the discussion, would you prefer the config to have a style enum akin to this to replace tarmac: bool?

~ config.rs

#[derive(Debug, Deserialize, Serialize, PartialEq)]
#[serde(rename_all = "snake_case")]
pub enum StyleType {
    Flat,
    Nested
}

In the config I would add a style option, and it would end up looking like this in asphalt.toml

style = "nested"

~ state.rs

pub struct State {
    ...
    pub style: StyleType,
    ...
}

@jackTabsCode
Copy link
Owner

Yeah, that should work. Let's make sure it's optional and Flat is the default.

@bibi-reden
Copy link
Contributor Author

I've attempted to implement the concept in the previous comment with this commit: 1757304

@jackTabsCode
Copy link
Owner

Nice. Can we convert .expect calls to .context to better fit in with the rest of the codebase?

@bibi-reden
Copy link
Contributor Author

Gotcha! It is done.

@jackTabsCode jackTabsCode merged commit 81e85d1 into jackTabsCode:main Apr 26, 2024
4 checks passed
@jackTabsCode
Copy link
Owner

Thank you. I'm not going to release quite yet-going to restructure the config file a little bit, then I'll push out 0.5.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants