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

When a directory has a dot (".") in it, but the filename doesn't, FileNotFoundException results. #300

Closed
mtnygard opened this Issue Aug 16, 2016 · 8 comments

Comments

Projects
None yet
3 participants
@mtnygard

Situation

My content files are supplied in asciidoc. I have output.extension and outfilesuffix both set to nothing in jbake.properties.

The target directory has a dot in it. Inside org.jbake.app.Renderer, the output file path is truncated at the dot. The renderer then attempts to create a writer on that path. The path happens to refer to a directory, so it passes the file.exists() check, but it is a directory so it can't be opened for writing.

Expected behavior

Using String.lastIndexOf() to get the file extension is not sufficient. It should only look for the last dot after the last file separator character.

mtnygard added a commit to mtnygard/jbake that referenced this issue Aug 16, 2016

mtnygard added a commit to mtnygard/jbake that referenced this issue Aug 16, 2016

@mtnygard

This comment has been minimized.

Show comment
Hide comment
@mtnygard

mtnygard Aug 16, 2016

I have a fix for this, but am having a hard time crafting a test. It needs an output directory with a dot in it (which AbstractTemplateEngineRenderingTest doesn't have in its setup) and a content file without an extension (which the Parser doesn't know how to handle in the tests.)

So far, I've just copied the renderPost test case. (Visible in mtnygard/jbake@656a39c).

Got any ideas about how to make this less cumbersome?

I have a fix for this, but am having a hard time crafting a test. It needs an output directory with a dot in it (which AbstractTemplateEngineRenderingTest doesn't have in its setup) and a content file without an extension (which the Parser doesn't know how to handle in the tests.)

So far, I've just copied the renderPost test case. (Visible in mtnygard/jbake@656a39c).

Got any ideas about how to make this less cumbersome?

@jonbullock

This comment has been minimized.

Show comment
Hide comment
@jonbullock

jonbullock Sep 26, 2016

Member

Hmm the template engine tests don't lend themselves well to this situation.

For this situation I'd create a separate test case (maybe even a new class) that just tests the Renderer.render method for this specific scenario. We've recently added support into the tests for Mockito which you could use.

Member

jonbullock commented Sep 26, 2016

Hmm the template engine tests don't lend themselves well to this situation.

For this situation I'd create a separate test case (maybe even a new class) that just tests the Renderer.render method for this specific scenario. We've recently added support into the tests for Mockito which you could use.

@jonbullock

This comment has been minimized.

Show comment
Hide comment
@jonbullock

jonbullock Sep 26, 2016

Member

I'm working on v2.5.0 at the moment so if you raise a PR with the fix in I'd be happy to include it in the next release, the test case can always come after in a separate PR.

Member

jonbullock commented Sep 26, 2016

I'm working on v2.5.0 at the moment so if you raise a PR with the fix in I'd be happy to include it in the next release, the test case can always come after in a separate PR.

@jonbullock jonbullock added this to the v2.5.1 milestone Jan 28, 2017

@jonbullock

This comment has been minimized.

Show comment
Hide comment
@jonbullock

jonbullock Jan 30, 2017

Member

Come up with this test for the bug: https://gist.github.com/jonbullock/41dde7da607afe3a78983c984f288693

It requires a new constructor to be added to Renderer which may help other tests long term, the test focuses on testing the logic within Renderer as there are some scenario's that don't have any coverage at the moment so this test class can be expanded upon later on. Comments?

Member

jonbullock commented Jan 30, 2017

Come up with this test for the bug: https://gist.github.com/jonbullock/41dde7da607afe3a78983c984f288693

It requires a new constructor to be added to Renderer which may help other tests long term, the test focuses on testing the logic within Renderer as there are some scenario's that don't have any coverage at the moment so this test class can be expanded upon later on. Comments?

@ancho

This comment has been minimized.

Show comment
Hide comment
@ancho

ancho Jan 30, 2017

Member

Hey Jon,

I think it is good style to make the DelegatingTemplateEngine configurable. Testing is much easier that way.

But I wonder if destination and templatesPath is really necessary, as those two attributes should be present within the CompositeConfiguration accessible with config.getString(Keys.DESTINATION_FOLDER) and config.getString(Keys.TEMPLATE_FOLDER) .

Member

ancho commented Jan 30, 2017

Hey Jon,

I think it is good style to make the DelegatingTemplateEngine configurable. Testing is much easier that way.

But I wonder if destination and templatesPath is really necessary, as those two attributes should be present within the CompositeConfiguration accessible with config.getString(Keys.DESTINATION_FOLDER) and config.getString(Keys.TEMPLATE_FOLDER) .

@ancho

This comment has been minimized.

Show comment
Hide comment
@ancho

ancho Jan 30, 2017

Member

How about introducing a JbakeConfiguration ?

See https://gist.github.com/ancho/f61643f93d6bdb9a73c90dca32202d3e

Member

ancho commented Jan 30, 2017

How about introducing a JbakeConfiguration ?

See https://gist.github.com/ancho/f61643f93d6bdb9a73c90dca32202d3e

@jonbullock

This comment has been minimized.

Show comment
Hide comment
@jonbullock

jonbullock Jan 31, 2017

Member

DelegatingTemplateEngine needs refactoring as well really as that is what needs templatesPath in the real life usage scenario. At the very least this is a step in the right direction and makes sure this bug is fixed and tested for.

If the user hasn't specified a custom source and destination we could get them from the Config and I noticed you suggest a solution for this (by setting it into the Config during load) in your gist.

I could swear again there was an action to convert the Config over to a more structured class like you have proposed but I can't find it either. It makes perfect sense to do this... so could you raise a separate issue for this? It's quite an invasive change to be made and I'd like to do it after we move to Gradle and split up the project into sub-modules.

Member

jonbullock commented Jan 31, 2017

DelegatingTemplateEngine needs refactoring as well really as that is what needs templatesPath in the real life usage scenario. At the very least this is a step in the right direction and makes sure this bug is fixed and tested for.

If the user hasn't specified a custom source and destination we could get them from the Config and I noticed you suggest a solution for this (by setting it into the Config during load) in your gist.

I could swear again there was an action to convert the Config over to a more structured class like you have proposed but I can't find it either. It makes perfect sense to do this... so could you raise a separate issue for this? It's quite an invasive change to be made and I'd like to do it after we move to Gradle and split up the project into sub-modules.

@jonbullock jonbullock self-assigned this Jan 31, 2017

@ancho

This comment has been minimized.

Show comment
Hide comment
@ancho

ancho Feb 1, 2017

Member

Sure. It's on my list....

Member

ancho commented Feb 1, 2017

Sure. It's on my list....

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment