Skip to content

Fix: --source-type is now honoured for stdin and for existing non-.java files#2048

Merged
maxandersen merged 11 commits intojbangdev:mainfrom
lutzleonhardt:fix/#1975-source-typ-not-honored
Jul 2, 2025
Merged

Fix: --source-type is now honoured for stdin and for existing non-.java files#2048
maxandersen merged 11 commits intojbangdev:mainfrom
lutzleonhardt:fix/#1975-source-typ-not-honored

Conversation

@lutzleonhardt
Copy link
Copy Markdown
Contributor

@lutzleonhardt lutzleonhardt commented Jun 6, 2025

Closes #1975

🐞 Problem

Scenario Expected Current behaviour
pbpaste | jbang --source-type=java - Treated as Java source Piped script cached as .jsh and executed via JShell
jbang --source-type=java myscript.jsh Compiled as Java (or rejected) File copied as myscript.jsh.javajavac “bad file name” error

🔧 What changed

  • LiteralScriptResourceResolver
    • Accepts the forceType parameter and always prefers it over auto-detection when caching stdin.
    • Keeps the existing “prepend underscore if the name starts with a digit” safeguard.
  • RenamingScriptResourceResolver
    • When --source-type is supplied, we strip the original extension before adding the forced one
      (myscript.jshmyscript.java instead of myscript.jsh.java).

✅ Tests

  • Added TestSourceType with two cases:

    1. stdinHonoursForceType() – verifies that piping to - is handled as Java when requested.
    2. forceJavaOnJshFile() – verifies that forcing Java on a .jsh file succeeds and does not generate a .jsh.jar.
  • Manual test
    image

♻ Regression risk

Localised to two resolvers; no logic elsewhere changed. Existing behaviour for regular .java, .jsh, remote, and gav resources stays untouched.


🙌 Credits

Most of the root-cause analysis was assisted by Brokk AI.
If you’re curious, I recorded the full debugging session:
https://youtu.be/t_7MqowT638?si=QVySbqoy0mqLcljz
(24 min video – optional to watch, of course).


📋 Checklist

  • Code builds and all existing tests pass
  • New unit-tests added
  • --enable-preview workflow verified manually
  • Release notes updated (if required)

Summary by Sourcery

Fix handling of --source-type for piped input and existing non-.java scripts by propagating the forced source type through both LiteralScriptResourceResolver and RenamingScriptResourceResolver, and add tests to verify the corrected behavior.

Bug Fixes:

  • Honor --source-type when caching stdin scripts instead of defaulting to .jsh
  • Strip existing file extensions before applying a forced source type to avoid generating ".jsh.java" filenames

Enhancements:

  • Allow LiteralScriptResourceResolver to accept and use a forceType parameter for consistent type enforcement

Tests:

  • Add TestSourceType to verify enforced Java source type for stdin input and on .jsh files

@sourcery-ai
Copy link
Copy Markdown
Contributor

sourcery-ai Bot commented Jun 6, 2025

Reviewer's Guide

This PR introduces a forceType override for both stdin-based and existing non-.java scripts by extending the resource resolvers to honor --source-type, updating caching and renaming logic accordingly, and adds tests to validate the new behavior.

File-Level Changes

Change Details Files
Enable --source-type override for stdin in LiteralScriptResourceResolver
  • Add forceType field and constructor to resolver
  • Pass forceType from ProjectBuilder to LiteralScriptResourceResolver
  • Overload stringToResourceRef to accept forceType
  • Adjust suffix and basename logic to prefer forceType and prefix underscore for invalid Java identifiers
src/main/java/dev/jbang/source/resolvers/LiteralScriptResourceResolver.java
src/main/java/dev/jbang/source/ProjectBuilder.java
Strip original extension before appending forced type in RenamingScriptResourceResolver
  • When forceType is set, remove existing file extension using regex
  • Append the forced extension instead of simply adding to the original name
  • Retain fallback behavior when no forceType is provided
src/main/java/dev/jbang/source/resolvers/RenamingScriptResourceResolver.java
Add unit tests for source-type enforcement
  • Test that piping code via stdin honors --source-type=java without using JShell
  • Verify forcing Java on a .jsh file succeeds and does not produce a .jsh.jar
src/test/java/dev/jbang/cli/TestSourceType.java

Assessment against linked issues

Issue Objective Addressed Explanation
#1975 Ensure that when running a script from stdin with jbang --source-type=java -, the script is treated as Java source code and not as a JShell script.
#1975 Ensure that when running jbang --source-type=java myscript.jsh, the script is compiled as Java (or rejected if it's not valid Java) and not treated as a JShell script.

Possibly linked issues


Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

Copy link
Copy Markdown
Contributor

@sourcery-ai sourcery-ai Bot left a comment

Choose a reason for hiding this comment

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

Hey @lutzleonhardt - I've reviewed your changes and they look great!

Here's what I looked at during the review
  • 🟡 General issues: 1 issue found
  • 🟢 Security: all looks good
  • 🟡 Testing: 1 issue found
  • 🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment thread src/test/java/dev/jbang/cli/TestSourceType.java Outdated
Copy link
Copy Markdown
Contributor

@quintesse quintesse left a comment

Choose a reason for hiding this comment

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

Looks really good, just a couple of small changes and questions.

Comment thread src/main/java/dev/jbang/source/resolvers/RenamingScriptResourceResolver.java Outdated
Comment thread src/main/java/dev/jbang/source/resolvers/LiteralScriptResourceResolver.java Outdated
Comment thread src/main/java/dev/jbang/source/resolvers/LiteralScriptResourceResolver.java Outdated
Comment thread src/main/java/dev/jbang/source/resolvers/LiteralScriptResourceResolver.java Outdated
Comment thread src/test/java/dev/jbang/cli/TestSourceType.java Outdated
@lutzleonhardt lutzleonhardt force-pushed the fix/#1975-source-typ-not-honored branch from 884e0ba to 2a1d492 Compare June 6, 2025 22:27
@lutzleonhardt
Copy link
Copy Markdown
Contributor Author

@quintesse Thanks for your feedback. I have implemented your suggestions so far. Let me know if something is wrong or missing -thx.

Copy link
Copy Markdown
Contributor

@quintesse quintesse left a comment

Choose a reason for hiding this comment

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

Still some minor changes to be made, but almost there!

Comment thread src/main/java/dev/jbang/util/Util.java Outdated
Comment thread src/main/java/dev/jbang/util/Util.java Outdated
Comment thread src/test/java/dev/jbang/cli/TestRun.java Outdated
Comment thread src/main/java/dev/jbang/source/resolvers/LiteralScriptResourceResolver.java Outdated
Comment thread src/main/java/dev/jbang/source/resolvers/RenamingScriptResourceResolver.java Outdated
Comment thread src/test/java/dev/jbang/util/TestUtil.java Outdated
Comment thread src/test/java/dev/jbang/cli/TestRun.java Outdated
@lutzleonhardt
Copy link
Copy Markdown
Contributor Author

lutzleonhardt commented Jun 7, 2025

Let's iterate on it until it's satisfying :) Thanks for you input, let me know when something is missing

@lutzleonhardt lutzleonhardt requested a review from quintesse June 7, 2025 12:54
@lutzleonhardt lutzleonhardt force-pushed the fix/#1975-source-typ-not-honored branch 2 times, most recently from 6df4f37 to 4ac565d Compare June 7, 2025 19:31
@quintesse quintesse force-pushed the fix/#1975-source-typ-not-honored branch from 4ac565d to 504d68f Compare June 11, 2025 09:51
@lutzleonhardt lutzleonhardt force-pushed the fix/#1975-source-typ-not-honored branch from 504d68f to 1d047e4 Compare June 12, 2025 22:05
@lutzleonhardt
Copy link
Copy Markdown
Contributor Author

@quintesse Anything missing here from my side? Thanks

@quintesse
Copy link
Copy Markdown
Contributor

quintesse commented Jun 13, 2025

@lutzleonhardt if you have the time you can take a look at why the IT tests are failing. If not I'll look into it as soon as aI have some time.

@maxandersen
Copy link
Copy Markdown
Collaborator

just testing this and at first glance it solves the reported bug (good) but it also introduce a regression and fail at handling this:

jbang --code "$(cat itests/helloworld.java)" jbangtest

it does not save as a helloworld.java file but instead _helloworld.java which worked previously. Thats what the failed itests is about.

Comment thread src/main/java/dev/jbang/util/Util.java
@maxandersen
Copy link
Copy Markdown
Collaborator

I've added code to detect identifers so it wont add _ unless really need to. wdyt?

@quintesse wiremock generated new files from foojay. expected i guess?

Comment thread src/main/java/dev/jbang/util/Util.java Outdated
@lutzleonhardt lutzleonhardt force-pushed the fix/#1975-source-typ-not-honored branch from 004ed90 to 2a8a595 Compare June 23, 2025 07:07
@maxandersen
Copy link
Copy Markdown
Collaborator

the pr was blocked/failing due to bad formatting. pushed update with that done.

Copy link
Copy Markdown
Collaborator

@maxandersen maxandersen left a comment

Choose a reason for hiding this comment

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

lgtm

@maxandersen maxandersen merged commit 5f835de into jbangdev:main Jul 2, 2025
9 of 10 checks passed
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.

source-type not honored

3 participants