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

[Feature] Allow // as a comment that stays in compiled file #403

Merged

Conversation

Ph0enixKM
Copy link
Member

  • Comment is now translated to a bash comment
  • Added --release flag to optimise resulting code for production. For now it just removes the comments

@Ph0enixKM Ph0enixKM self-assigned this Aug 12, 2024
@Ph0enixKM Ph0enixKM linked an issue Aug 12, 2024 that may be closed by this pull request
@Ph0enixKM Ph0enixKM marked this pull request as ready for review August 12, 2024 18:30
@mks-h
Copy link
Member

mks-h commented Aug 12, 2024

Maybe it is a good idea to use --minify flag, instead. I can imagine a future scenario where amber code is being used as a bash library, and it would be helpful to have comments for people who'd like to inspect it. Therefore removing comments might not be a good idea for an otherwise optimized release build.

@Ph0enixKM
Copy link
Member Author

@mks-h You're right. Should the doc comment be removed too when the minify flag is present?

@mks-h
Copy link
Member

mks-h commented Aug 13, 2024

I'd say yes, minified file should only have what's necessary to run — not a character extra.

@Ph0enixKM
Copy link
Member Author

Ph0enixKM commented Aug 18, 2024

@mks-h But then that will impact the intellisense (provided by LSP) if someone hands you over a library in a minified version. Let's merge just this and mangle just the inner variables. How about that?

@mks-h
Copy link
Member

mks-h commented Aug 18, 2024

Sure, sounds good to me

@b1ek
Copy link
Member

b1ek commented Aug 19, 2024

I'd say yes, minified file should only have what's necessary to run — not a character extra.

i thought we want to make it so that the generated bash file should be readable to some extent so that the user could review it before running. ref: #227

@b1ek
Copy link
Member

b1ek commented Aug 19, 2024

tbh id rather leave the comments in the generated code and have the user remove them manually with sed 's/#.*//' or something, if they want to minify it

@Mte90
Copy link
Member

Mte90 commented Aug 19, 2024

If we require that users to run other commands to process the script generated is not a common behavior with scripting languages.
We are doing a transpiler and those stuff should be implemented natively as the user doesn't do anything else that just execute the program.

@Ph0enixKM Ph0enixKM merged commit 7a6f90c into master Aug 19, 2024
1 check passed
@Ph0enixKM Ph0enixKM deleted the 106-feature-allow-as-a-comment-that-stays-in-compiled-file branch August 19, 2024 18:18
@mks-h
Copy link
Member

mks-h commented Aug 19, 2024

i thought we want to make it so that the generated bash file should be readable to some extent

For development and production builds — yes, it'd be great to have them readable. But minimization is a separate optional step, which should be able to run over both development and production builds. Its purpose is to represent given code in as little amount of characters as possible without modifying the way it runs or functions — basically compressing it by omitting unnecessary characters.

have the user remove them manually

Thinking back on it, I'm not opposed to have minimization separate from the compiler, considering it most likely cannot gain anything from access to amber's inner-workings. That being said, having the --minimize flag would still be useful, even if it just runs some third-party bash minimizer under the hood.

Mte90 pushed a commit to Mte90/Amber that referenced this pull request Sep 19, 2024
…ng#403)

* feat(comment): persist regular comments in bash code and add release flag to remove them

* fix(comment): Resolve clippy linting issyes

* fix(cc-flag): rename release to minify

* fix(minify): remove weird minify file
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.

[Feature] Allow // as a comment that stays in compiled file
4 participants