-
Notifications
You must be signed in to change notification settings - Fork 14
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
Minor fix for tika validation. #96
base: MOODLE_310_STABLE
Are you sure you want to change the base?
Conversation
Adding a CLI script for testing tika too.
|
||
Options: | ||
-h, --help Print out this help | ||
-t, --testfileid (Optional) PDF or accepted file id to send to tika for analysis |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where do we get testfileid from? Is it id from mdl_files? If so, then this is not user friendly. Maybe we can pass a path to a test file instead? Or we could use files from tests/fixtures?
// along with Moodle. If not, see <http://www.gnu.org/licenses/>. | ||
|
||
/** | ||
* CLI config tester |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you considered move this CLI to UI instead? E.g. after submitting setting we could check if it actually works and then display results?
Thanks @dvdcastro for submitting the patch. Bug fix looks good, however, I'm a bit hesitant about a new CLI script. I'd suggest to move this check to UI. If it's not something that you can do in a near future we can split bugfix and a new check as different PR so the bugfix could land faster. |
I merged fixing part of this PR here #105 |
Adding a CLI script for testing tika too.