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.

Sign up for GitHub

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

Jump to bottom

feat: enable integrations to run externally #1290

Merged
merged 4 commits into from
Mar 13, 2022
Merged

feat: enable integrations to run externally #1290

merged 4 commits into from
Mar 13, 2022

Conversation

quintesse
Copy link
Contributor

Fixes #1084

@quintesse quintesse requested a review from maxandersen March 10, 2022 22:46
@quintesse quintesse mentioned this pull request Mar 10, 2022
Closed
@codecov
Copy link

codecov bot commented Mar 10, 2022 •
edited
Loading

Codecov Report

Merging #1290 (3b9af91) into main (d3e726c) will decrease coverage by 0.53%.
The diff coverage is 28.39%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #1290      +/-   ##
============================================
- Coverage     59.35%   58.81%   -0.54%     
- Complexity     1176     1179       +3     
============================================
  Files            98       99       +1     
  Lines          6052     6102      +50     
  Branches       1005     1014       +9     
============================================
- Hits           3592     3589       -3     
- Misses         1951     2007      +56     
+ Partials        509      506       -3
Flag Coverage Δ
Linux 57.52% <28.39%> (-0.53%) ⬇️
Windows 57.65% <28.39%> (-0.53%) ⬇️

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

Impacted Files Coverage Δ
src/main/java/dev/jbang/spi/IntegrationInput.java 0.00% <0.00%> (ø)
src/main/java/dev/jbang/spi/IntegrationResult.java 50.00% <0.00%> (-50.00%) ⬇️
...rc/main/java/dev/jbang/spi/IntegrationManager.java 18.24% <15.38%> (-3.12%) ⬇️
src/main/java/dev/jbang/source/Source.java 82.20% <92.85%> (+0.33%) ⬆️
...in/java/dev/jbang/source/builders/BaseBuilder.java 69.03% <100.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 d3e726c...3b9af91. Read the comment docs.

@quintesse
Copy link
Contributor Author

Btw @maxandersen , I don't know if the new code (since the last big refactor) also fixes this issue: #1143 , but the issue isn't very clear so I'm not sure really how to test it. Perhaps you can take a look? Or come up with a better test/description?

@maxandersen
Copy link
Collaborator

Nice! Is it so it always runs externally or you think we can try be smart only do it when needed ?

@quintesse
Copy link
Contributor Author

As the commit message explains, we're smart about it 🙂

Checks if the requested Java version is the same as the one JBang is
using and if not it will start a sub process to handle the integration
with the correct JDK.

@maxandersen
Copy link
Collaborator

this is brilliant fix - opens up for also run it when/if we native compile jbang!

about #1143 is not solved nor related to this afaics.
If you take the command in the issue and run it gives same error.

maxandersen
maxandersen requested changes Mar 13, 2022
View reviewed changes
Copy link
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.

please rebase it and we can happily merge it!

@quintesse
Copy link
Contributor Author

Rebased.

@quintesse
chore: Added getTags() to Source
b733418 
Is a pre-filtered version of `getLines()` that only returns Lines
that start with `//`.
@quintesse
refactor: removed unnecessary code
e01bcd0
@quintesse
refactor: extacted code for single integration execution
c4103b9 
The code that runs a single integration is now sepearate and its input
and ouput have been turned into serializable objects. This prepares
for the next step where those integrations can be run externally.
@quintesse
feat: enable integrations to run externally
3b9af91 
Checks if the requested Java version is the same as the one JBang is
using and if not it will start a sub process to handle the integration
with the correct JDK.

Fixes jbangdev#1084
@maxandersen
Copy link
Collaborator

Build error?

@quintesse
Copy link
Contributor Author

Sorry, forgot to push :-)

@maxandersen maxandersen merged commit b44d783 into jbangdev:main Mar 13, 2022
@maxandersen maxandersen mentioned this pull request Mar 19, 2022
Closed
@maxandersen maxandersen mentioned this pull request Oct 15, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maxandersen maxandersen maxandersen requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

jbang --java 17 when default is java 11 build integration fails
2 participants
@quintesse @maxandersen