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

feat: cli can take custom alphabet as args #116

Merged
merged 6 commits into from
Sep 20, 2024

Conversation

Phaired
Copy link
Contributor

@Phaired Phaired commented Sep 18, 2024

(Maintainer's note - this is a counterpart to #100 for the CLI tool)

Copy link
Owner

@robertknight robertknight left a comment

Choose a reason for hiding this comment

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

Thanks for the patch. I requested some adjustment to the wording to make it clear that the alphabet has to match the model. The user can't just use whatever alphabet they like.

In future I think it will make sense to have a way to embed the alphabet in the model file, or provide it as part of a configuration file that is shipped alongside the model. In the interim, adding a CLI argument is useful.

ocrs-cli/src/main.rs Outdated Show resolved Hide resolved
ocrs-cli/src/main.rs Outdated Show resolved Hide resolved
ocrs-cli/src/main.rs Outdated Show resolved Hide resolved
@Phaired
Copy link
Contributor Author

Phaired commented Sep 20, 2024

I agree that embedding the alphabet in the model file would be a good feature.
Sorry for the many commits.

@robertknight
Copy link
Owner

Sorry for the many commits.

That's not a problem. As a maintainer, I can also do this when merging a PR using the "Squash and merge" option.

For future reference, you can use git rebase --interactive followed by git push --force to tidy up the commit history by editing commit messages, merging commits or other tasks.

@robertknight robertknight merged commit 1317bb9 into robertknight:main Sep 20, 2024
2 checks passed
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.

2 participants