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

fix: //SOURCES now work in .jsh files too #1221

Merged
merged 3 commits into from
Feb 2, 2022

Conversation

quintesse
Copy link
Contributor

Fixes #1220

@quintesse quintesse requested a review from maxandersen January 26, 2022 22:08
@codecov
Copy link

codecov bot commented Jan 26, 2022

Codecov Report

Merging #1221 (c4eaabe) into main (91fbc89) will decrease coverage by 0.16%.
The diff coverage is 47.36%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #1221      +/-   ##
============================================
- Coverage     59.00%   58.84%   -0.17%     
- Complexity     1050     1058       +8     
============================================
  Files            84       86       +2     
  Lines          5579     5666      +87     
  Branches        937      956      +19     
============================================
+ Hits           3292     3334      +42     
- Misses         1805     1839      +34     
- Partials        482      493      +11     
Flag Coverage Δ
Linux 57.69% <47.36%> (-0.15%) ⬇️
Windows 57.60% <47.36%> (-0.15%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/main/java/dev/jbang/catalog/Catalog.java 70.00% <ø> (ø)
src/main/java/dev/jbang/cli/Catalog.java 32.38% <0.00%> (ø)
.../main/java/dev/jbang/catalog/TemplateProperty.java 27.77% <27.77%> (ø)
src/main/java/dev/jbang/cli/Template.java 47.61% <43.85%> (-0.21%) ⬇️
src/main/java/dev/jbang/catalog/CatalogUtil.java 61.78% <50.00%> (ø)
.../java/dev/jbang/cli/TemplatePropertyConverter.java 80.00% <80.00%> (ø)
src/main/java/dev/jbang/cli/Run.java 74.25% <83.33%> (+0.14%) ⬆️
src/main/java/dev/jbang/catalog/Template.java 48.00% <100.00%> (+2.16%) ⬆️
src/main/java/dev/jbang/util/Util.java 60.43% <0.00%> (-0.22%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 51f1619...c4eaabe. Read the comment docs.

@@ -194,6 +194,8 @@ String generateCommandLine(Source src, RunContext ctx) throws IOException {
}
}

optionalArgs.add("--execution=local");
Copy link
Collaborator

Choose a reason for hiding this comment

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

what is that flag trying to achieve?

Copy link
Collaborator

Choose a reason for hiding this comment

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

like, my jshell has no documentation for that flag afaics but it does not complain about it...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

jshell, by default, starts up a VM in the background to which it connects. But this means you can't read stdin from that VM because it's not connected to the terminal. This option tells jshell to not do that and just use the local process for the VM, which then inherits the stdin from jshell. It even makes jshell a bit faster it seems.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmmm...interesting...what's the downside ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure, the information on this is very limited. I'm assuming it's more efficient when you call jshell a lot.

Copy link
Collaborator

Choose a reason for hiding this comment

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

i'm hesitant to do this specific change but lets merge in and we can revert if some usecase fails

@maxandersen
Copy link
Collaborator

should add a line or two about this behavior in the docs.

The semantics is that they are treated as includes or?

@quintesse
Copy link
Contributor Author

should add a line or two about this behavior in the docs.

What more do you want to say about it that the current docs aren't already explaining?

"The main script file defines all the dependencies and you add more source files into the application using //SOURCES <filename>. If included source has //SOURCES that will also get included recursively."

The current explanation is a bit vague perhaps but I think it's best not to go into too much technical detail. The sources just get included into the application, no need to know exactly how. Right?

@maxandersen
Copy link
Collaborator

Well; for java the ordering does not really matter as the classes just gets added to jar.

In jshell ordering matters if they are treated as inclusion and executed in order.

Basically if that is the semantics the description needs to be more specific than its vague now.

@quintesse
Copy link
Contributor Author

Ok, added a bit of explanation

@quintesse quintesse requested a review from maxandersen February 2, 2022 17:04
@maxandersen maxandersen merged commit 3367791 into jbangdev:main Feb 2, 2022
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 doesn't do anything for .jsh scripts
2 participants