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

Use separate config properties for adapter enable/disable lists #256

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

lafrenierejm
Copy link
Contributor

The adapters property in the configuration file is replaced by two new properties, adapters_enable and adapters_disable. The CLI is unchanged; the new properties are only supported in the config file and can be overridden by the --adapters CLI option.

This is intended to resolve #183 by making it clearer how to enable or disable adapters via the config file.

@lafrenierejm
Copy link
Contributor Author

This builds on #250. I would recommend reviewing that PR before this one.

@phiresky
Copy link
Owner

phiresky commented Oct 8, 2024

This could be considered a breaking change wrt to people's config files, which we should probably care about now. I also think it's a good goal to keep the config and CLI as aligned as possible, so I'm not sure if this is the best approach

@phiresky
Copy link
Owner

phiresky commented Oct 8, 2024

Maybe a leaner solution would be to just (significantly) improve the docs for this parameter

@phiresky
Copy link
Owner

phiresky commented Oct 8, 2024

To expand on the config breakage:

The generated json-schema is versioned but this change means we would have to actually implement a method to update it in some way or have read-compatibility with older versions. I envisioned something like the config.json file specifies the schema, and if a config file in an old schema is found it is automatically rewritten to the new format on first read, while the .schema.json files are written next to each other.
I left that part out so far since it's complex, with the hope we could just keep everything backwards compatible for as long as possible.

@lafrenierejm
Copy link
Contributor Author

This could be considered a breaking change wrt to people's config files, which we should probably care about now.

As a counterargument: If breaking changes are going to be made, doing so now (before stable 1.0.0 is released) is the best time to do it.

@lafrenierejm
Copy link
Contributor Author

I also think it's a good goal to keep the config and CLI as aligned as possible, so I'm not sure if this is the best approach

That's fair. I find this PR's approach of separate enable/disable properties to be much more intuitive than the current syntax, so my preference would be:

  1. Keep this PR's config file changes as-is, but expose the enable/disable lists as new CLI options.
  2. Mark the current --rga-adapters CLI option deprecated.
  3. Remove the --rga-adapters CLI option prior to the 1.0.0 release.

That said, I recognize that this is largely a matter of personal preference. In the end the call is yours to make.

@lafrenierejm
Copy link
Contributor Author

lafrenierejm commented Oct 14, 2024

To expand on the config breakage:

The generated json-schema is versioned but this change means we would have to actually implement a method to update it in some way or have read-compatibility with older versions. I envisioned something like the config.json file specifies the schema, and if a config file in an old schema is found it is automatically rewritten to the new format on first read, while the .schema.json files are written next to each other. I left that part out so far since it's complex, with the hope we could just keep everything backwards compatible for as long as possible.

I do wonder if there's an existing Crate that provides some of that functionality. But I agree that everything related to the config versioning is low priority at least until 1.0.0 is released.

@lafrenierejm
Copy link
Contributor Author

I also think it's a good goal to keep the config and CLI as aligned as possible, so I'm not sure if this is the best approach

That's fair. I find this PR's approach of separate enable/disable properties to be much more intuitive than the current syntax, so my preference would be:

1. Keep this PR's config file changes as-is, but expose the enable/disable lists as new CLI options.

2. Mark the current `--rga-adapters` CLI option deprecated.

3. Remove the `--rga-adapters` CLI option prior to the 1.0.0 release.

@phiresky What are your thoughts on this approach?

@phiresky
Copy link
Owner

phiresky commented Dec 8, 2024

It sounds pretty reasonable.. though with your new approach you have no option to fully override the adapters list. As in, rga --rga-adapters=pdf is not possible (disable all default adapters and only use pdf). That's why I chose that "weird" syntax, because it's powerful..

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.

Can't disable adapters
2 participants