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: Unable to use {basename} in template file-refs #1319

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

nandorholozsnyak
Copy link
Contributor

- {filename} and {basename} have different validation rules for their usage
@codecov
Copy link

codecov bot commented Mar 20, 2022

Codecov Report

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

❗ 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%) ⬇️
Windows 56.05% <100.00%> (-0.02%) ⬇️

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%) ⬇️

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.

- Test for case when {basename} is being used in a template file ref
- fixes jbangdev#1318
@nandorholozsnyak
Copy link
Contributor Author

Can I ask for a review @quintesse @maxandersen ?

Tests that the name given by the user matches the template
@quintesse
Copy link
Contributor

quintesse commented Mar 21, 2022

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 quintesse self-requested a review March 21, 2022 22:04
Copy link
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.

See comment I left on PR

@quintesse
Copy link
Contributor

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.

@nandorholozsnyak
Copy link
Contributor Author

Wow.

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

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.

Unable to use {basename} in template file-refs
2 participants