Skip to content

Remove unused is_match function - #898

Merged
NSoiffer merged 1 commit into
daisy:mainfrom
moritz-gross:inline--is-match
Oct 24, 2026
Merged

NSoiffer merged 1 commit into
daisy:mainfrom
moritz-gross:inline--is-match

Conversation

@moritz-gross

Copy link
Copy Markdown
Collaborator

not sure what is happening here. to me it looks like this function does practically nothing.

  • I don't think the early return is every hit the way
  • The commented-out debug prints likely should be removed
  • the matching at the end re-implements is_true

Generally, I have a hard time following the naming scheme in some parts of MathCAT.
As I see it, a SpeechPattern corresponds to what we normally call a "rule", right?
Do you think we should rename it to SpeechRule ?

@github-actions

Copy link
Copy Markdown
Linux library size: 0.59 MiB (-0.00%)
Revision Release liblibmathcat.so
Base (31fa5c6) 0.59 MiB (614,800 bytes)
PR (42ad841) 0.59 MiB (614,784 bytes)
Change -16 bytes (-0.00%)

Built with default features, Rust 1.96.0, and Ubuntu 24.04. Workflow run.

@NSoiffer

Copy link
Copy Markdown
Collaborator

The pattern is one rule (a specific pattern to be matched). SpeechRules is a collection of them. SpeechRulesWithContext adds in the prefs that are used by some rules (built from all the files that are read for ClearSpeak in en, etc). Maybe it makes sense to rename it, but SpeechRules are much more than just a vector of SpeechPatterns. So SpeechRule/SpeechRules is misleading.

Perhaps the bigger issue is that these are just for speech, but for braille also. So maybe "Speech" should be dropped?

As for the change... I was initially sceptical that the change was correct. But I can see now that callers to find_match have made sure the test is true. Perhaps it would be better as an assertion? However, it does clean the code up to get rid of it. The alternative would be to make the test an assertion and replace the next part with a call it is_true. I'll stick with this change though.

@NSoiffer
NSoiffer merged commit 6fb16fd into daisy:main Oct 24, 2026
14 checks passed
@github-project-automation github-project-automation Bot moved this from Triage to Done in MathCAT Project Board Oct 24, 2026
@moritz-gross

Copy link
Copy Markdown
Collaborator Author

even more broadly, the whole filename speech.rs is misleading as a lot of it is used for Braille I think?
maybe rule_engine.rs or similar instead?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants