jbang icon indicating copy to clipboard operation
jbang copied to clipboard

fix: Unable to use {basename} in template file-refs

Open nandorholozsnyak opened this issue 3 years ago • 5 comments

  • {filename} and {basename} have different validation rules for their usage
  • fixes #1318

nandorholozsnyak avatar Mar 20 '22 14:03 nandorholozsnyak

Codecov Report

Merging #1319 (84965d1) into main (e61fcb4) will decrease coverage by 0.01%. The diff coverage is 100.00%.

:exclamation: Current head 84965d1 differs from pull request most recent head 55fab25. Consider uploading reports for the commit 55fab25 to get more accurate results

@@             Coverage Diff              @@
##               main    #1319      +/-   ##
============================================
- Coverage     56.64%   56.62%   -0.02%     
+ Complexity     1154     1153       -1     
============================================
  Files            99       99              
  Lines          6128     6128              
  Branches       1019     1019              
============================================
- Hits           3471     3470       -1     
  Misses         2176     2176              
- Partials        481      482       +1     
Flag Coverage Δ
Linux 55.43% <100.00%> (-0.02%) :arrow_down:
Windows 56.05% <100.00%> (-0.02%) :arrow_down:

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

Impacted Files Coverage Δ
src/main/java/dev/jbang/cli/Init.java 84.76% <100.00%> (-0.96%) :arrow_down:

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 e61fcb4...55fab25. Read the comment docs.

codecov[bot] avatar Mar 20 '22 14:03 codecov[bot]

Can I ask for a review @quintesse @maxandersen ?

nandorholozsnyak avatar Mar 21 '22 19:03 nandorholozsnyak

I added an extra test that shows where your code causes problems. That test should already have existed of course but it didn't :-)

So it's not that we disagree with what you want to do, it's a very useful feature. But the way templates work is honestly a bit obscure. So right now a template needs at least one of these two things to be true:

  • there is at least a single entry named {filename}=somefile.ext[.qute] and the name given by user to the init command should have the same .ext extension as the file after the = sign
  • there is at least a single entry named {basename}.ext=somefile.someext[.qute] and the name given by user to the init command should have the same .ext extension as the file before the = sign

Right now JBang does this check for all files in a template, which is not really what you want because it prevents a template author from doing exactly what you want to do.

But we still need to make sure that at least one file follows one of these rules!

And right now your code only follows the first rule.

The code would have to be adapted in such a way that it checks that the first file it encounters that matches those rules is enough. So having:

{filename}=somefile.java
{basename}.md=somefile.md

is correct, but so is:

{basename}.java=somefile.java
{basename}.md=somefile.md

And in both cases JBang has to check that typing init myfile.java is correct and so is init myfile, but typing init myfile.foo or init myfile.md should throw an error.

Edit: so to make sure that this is clear: in reality a template should either only contain a single {filename} entry (and an arbitrary number of {basename} entries), or it should contain only {basename} entries in which case the first one is the one that will get the name the user typed on the command line.

quintesse avatar Mar 21 '22 22:03 quintesse

Perhaps the best thing to do would be to do is to split this code into two steps: https://github.com/jbangdev/jbang/blob/main/src/main/java/dev/jbang/cli/Init.java#L67-L76

The first step checks if the correct rules are followed and throws an error when it finds a problem. And then the second step would be the current code without the error checking.

quintesse avatar Mar 21 '22 22:03 quintesse

Wow.

Thanks for the input, let me dig deeper and find a good solution for this as soon as I have time.

nandorholozsnyak avatar Mar 22 '22 07:03 nandorholozsnyak