Skip to content

Conversation

@geoHeil
Copy link

@geoHeil geoHeil commented Aug 7, 2025

Resolves #362

@github-actions
Copy link
Contributor

github-actions bot commented Aug 7, 2025

DCO Check Passed

Thanks @geoHeil, all your commits are properly signed off. 🎉

@mergify
Copy link

mergify bot commented Aug 7, 2025

Merge Protections

Your pull request matches the following merge protections and will not be merged until they are valid.

🟢 Enforce conventional commit

Wonderful, this rule succeeded.

Make sure that we follow https://www.conventionalcommits.org/en/v1.0.0/

  • title ~= ^(fix|feat|docs|style|refactor|perf|test|build|ci|chore|revert)(?:\(.+\))?(!)?:

@geoHeil geoHeil force-pushed the fix/get_output_paths-relative-no-artifactdir branch from 01cb61e to e92f120 Compare August 7, 2025 14:29
@geoHeil geoHeil changed the title fix path generation in case of relative file paths without artifact directory fix: path generation in case of relative file paths without artifact directory Aug 7, 2025
Copy link
Contributor

@dolfim-ibm dolfim-ibm left a comment

Choose a reason for hiding this comment

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

There is something weird here, it looks like you are duplicating the function _get_output_paths()?

…directory

Signed-off-by: Georg Heiler <georg.kf.heiler@gmail.com>
@geoHeil geoHeil force-pushed the fix/get_output_paths-relative-no-artifactdir branch from e92f120 to 0bd7792 Compare August 7, 2025 14:32
@geoHeil
Copy link
Author

geoHeil commented Aug 7, 2025

There is something weird here, it looks like you are duplicating the function _get_output_paths()?

indeed - fix pushed now

Signed-off-by: Georg Heiler <georg.kf.heiler@gmail.com>
if artifacts_dir.is_absolute():
final_artifacts_dir = artifacts_dir
else:
final_artifacts_dir = filename.parent / artifacts_dir
Copy link

Choose a reason for hiding this comment

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

I'm not sure about this line. Depending on from where this function is called, the use case can be different:

  • for example, if called from save_as_json() with image referenced flag, then one can have a use-case where images are stored completely separately from the json document itself, but path is still relative
  • the other example is when both save_as_json() and save_as_markdown() used, with image referenced flag, one could use the same artifact directory to store images and avoid duplication. To clarify, an example would be filename="../results/json/my_doc.json" and filename="../results/md/my_doc.md" with a shared reference images artifacts_dir="../results/ref_images/my_doc/"

Copy link

Choose a reason for hiding this comment

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

My proposal would be to not modify artifacts_dir in any way if value is provided.

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.

bug: relative asset path but no asset directory specified

3 participants