Skip to content

Change loading of external readers#1394

Open
bioball wants to merge 5 commits intoapple:mainfrom
bioball:external-reader-loading
Open

Change loading of external readers#1394
bioball wants to merge 5 commits intoapple:mainfrom
bioball:external-reader-loading

Conversation

@bioball
Copy link
Member

@bioball bioball commented Jan 6, 2026

This introduces breaking changes for external readers are loaded:

  1. In PklProject, relative paths are resolved relative to the enclosing PklProject file (make behavior consistent with how other settings work)
  2. Make CLI flags blow away any settings set on a PklProject
  3. Allow file URIs to be set as the executable

The overall goal is to make this behavior consistent with how other settings work.
For example, relative paths for other evaluator settings are already relative to the project directory.
Additionally, in every other case, CLI flags will overwrite any setting set within PklProject.

[windows]

This introduces breaking changes for external readers are loaded:

1. In PklProject, relative paths are resolved relative to the enclosing
   PklProject file (make behavior consistent with how other settings work)
2. Prevent external readers from being set for non-file based PklProject
3. Make CLI flags blow away any settings set on a PklProject

The overall goal is to make this behavior consistent with how other
settings work.
For example, relative paths for other evaluator settings are already
relative to the project directory.
Additionally, in every other case, CLI flags will overwrite any setting
set within PklProject.
@bioball bioball force-pushed the external-reader-loading branch from 2ada6e5 to 09218e7 Compare January 8, 2026 23:28
Comment on lines 123 to 125
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this used anywhere?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, nope, good catch!

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// Machiney for URI percent decoding.
/// Machinery for URI percent decoding.

Copy link
Contributor

@HT154 HT154 left a comment

Choose a reason for hiding this comment

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

LGTM modulo the above comments, approving to unblock

Copy link
Contributor

@stackoverflow stackoverflow left a comment

Choose a reason for hiding this comment

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

LGTM

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.

3 participants

Comments