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

added --customIndexTemplate option #477

Merged
merged 2 commits into from
Oct 13, 2021
Merged

added --customIndexTemplate option #477

merged 2 commits into from
Oct 13, 2021

Conversation

MananJethwani
Copy link
Contributor

@MananJethwani MananJethwani commented Aug 21, 2021

@kelson42 this PR will add --customIndexTemplate argument.

This depends on kiwix/libkiwix#607

@MananJethwani MananJethwani requested a review from kelson42 August 21, 2021 12:12
Copy link
Contributor

@kelson42 kelson42 left a comment

Choose a reason for hiding this comment

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

@MananJethwani
Copy link
Contributor Author

@kelson42 sorry, it wasn't working when I used -c it was working with --c, my bad.

Copy link
Contributor

@kelson42 kelson42 left a comment

Choose a reason for hiding this comment

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

The usage is not aligned properly. I would probably call the option only --customIndex.

If I give an invalid template file, then it should not start and print an appropriate error. For the moment, $ kiwix-serve --port=8080 --customIndexTemplate=phzh_core-greek-one_el_2020-12.zim phzh_core-greek-one_el_2020-12.zim starts and gives a blanck index page.

Same remark for a non-existing path.

Same remark for non-readable file.

@MananJethwani
Copy link
Contributor Author

@kelson42 the error for the non-readable file should be propagated by kiwix::getFileContent() in libkiwix/tools.h, so I couldn't do much but I am showing a warning if unreadable or empty template file is passed.
I have added errors for all other cases and exiting the code.

@kelson42
Copy link
Contributor

kelson42 commented Oct 2, 2021

@MananJethwani I don't know the code detail, but it should stop, not display a warning. Probably kiwix::getFileContent() should trigger an exception and you should catch it.

@mgautierfr
Copy link
Member

Agree with @kelson42 if the user pass a option and we cannot respect it, it should be a error.

Copy link
Member

@mgautierfr mgautierfr left a comment

Choose a reason for hiding this comment

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

Two really easy fix to do. But we are good.

This PR commits could be squash together in only one.

Need to kiwix/libkiwix#607 to be merged first.

Copy link
Member

@mgautierfr mgautierfr left a comment

Choose a reason for hiding this comment

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

We are good.
To merge once kiwix/libkiwix#607 is merged.

@kelson42 kelson42 marked this pull request as ready for review October 11, 2021 13:10
@kelson42 kelson42 merged commit 3df4a94 into master Oct 13, 2021
@kelson42 kelson42 deleted the kiwix-serve/issue/571 branch October 13, 2021 04:55
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