Skip to content

Adding JavaScript Preprocessor Support #10967

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

Merged
merged 1 commit into from
May 24, 2025
Merged

Conversation

zlonast
Copy link
Collaborator

@zlonast zlonast commented May 19, 2025

-optJSP was added to ghc similar to -optP for cpp.
I suggest supporting this change.
Implemented as discussed in #10722
Will close issue #10721

Template Α: This PR modifies behaviour or interface

@angerman
Copy link
Collaborator

I'm confused about the JSP naming scheme.
We have CC: C compiler, and CPP, C pre-processor.
Now we have JSP? Java Server Pages? JavaScript Processor? JavaScript Pre?
I'd argue this should be jspp/JSPP. It's for a JavaScript Pre-Processor.

I think the naming in GHC with optP, and optJSP is rather poor, but we have cppOptions in cabal, so we don't need to replicate the poor naming here and could use jsppOptions.

@geekosaur
Copy link
Collaborator

Ping @GulinSS re ghc naming??

@zlonast zlonast force-pushed the jsp-options branch 3 times, most recently from 0cacc4b to 8703783 Compare May 20, 2025 14:07
@GulinSS
Copy link
Contributor

GulinSS commented May 20, 2025

Hm, well, now I know why we use CPP and it is not a C++. 😄

Let's use anything what follow existing rules without copying jsp then. JSP was my first experience in GHC development participation and very probably I was not able to catch all details in one shot. Also I think that -optJSP is used rarely so we could try to schedule its renaming into a more appropriate variant.

@zlonast zlonast force-pushed the jsp-options branch 6 times, most recently from e1aa91a to 02ebc9c Compare May 21, 2025 08:19
Copy link
Collaborator

@mpickering mpickering left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed a few places which might need modifications. Looks like a good start, and thanks for the tests.

@zlonast
Copy link
Collaborator Author

zlonast commented May 21, 2025

I think now it looks the way I like it. The tests are written and are being passed :)

@zlonast
Copy link
Collaborator Author

zlonast commented May 21, 2025

@Mikolaj @Swordlash @phadej @angerman You might also be interested in this pull request 👀

@zlonast zlonast added this to the Considered for 3.16 milestone May 21, 2025
Copy link
Collaborator

@mpickering mpickering left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@Swordlash
Copy link
Contributor

Wow, thanks for picking this up.

Add JSPOptions tests

Add regressions tests

Co-authored-by: Mateusz Goslinowski <mateusz.goslinowski@gmail.com>
@zlonast zlonast added the merge me Tell Mergify Bot to merge label May 22, 2025
@mergify mergify bot added the ready and waiting Mergify is waiting out the cooldown period label May 22, 2025
@mergify mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label May 24, 2025
@mergify mergify bot merged commit 824735d into haskell:master May 24, 2025
55 checks passed
@zlonast zlonast deleted the jsp-options branch May 24, 2025 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API change merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days merge me Tell Mergify Bot to merge ready and waiting Mergify is waiting out the cooldown period
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants