Skip to content

Rename is_valid to is_empty in speech.rs - #899

Closed
moritz-gross wants to merge 1 commit into
daisy:mainfrom
moritz-gross:fix-misleading-name--is-valid
Closed

moritz-gross wants to merge 1 commit into
daisy:mainfrom
moritz-gross:fix-misleading-name--is-valid

Conversation

@moritz-gross

Copy link
Copy Markdown
Collaborator

while browsing speech.rs, I found this function name.
Being valid and empty should be semantically different imo, so I changed the name to just mirror is_empty.

@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 (570d670) 0.59 MiB (614,800 bytes)
Change 0 bytes (0.00%)

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

@NSoiffer

Copy link
Copy Markdown
Collaborator

I don't like this change. The two functions

    pub fn invalidate(&mut self) {
        self.ft.clear();
    }

    pub fn is_valid(&self) -> bool {
        self.ft.is_empty()
    }

form a pair. The real issue is that is_valid() is backwards (should return !self.ft.is_empty()) and the callers should have a different name (..._is_valid instead of ..._is_empty). Actually, it seems that is_valid() should be is_invalid(). That leaves the internals the same and the callers go from ...is_empty() to ...is_invalid().

I'm closing this. If you want to do the bigger renaming, open a new PR.

@NSoiffer NSoiffer closed this Oct 24, 2026
@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

ok then do you fix the naming in a way that makes sense?
The main thing that stood out to me that valid=empty seemed off to me.

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