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

docs: Correct return type of Search.find() #5711

Merged
merged 2 commits into from
Jan 13, 2025
Merged

Conversation

ferdnyc
Copy link
Contributor

@ferdnyc ferdnyc commented Jan 6, 2025

Description of changes:

The documentation comments for Search.find() claim it can return Range or false (changed from Range or boolean in 7e7f329), but neither is accurate. firstRange (the returned value) will always be either a Range, or null when the inner anonymous function returns false.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Pull Request Checklist:

@akoreman
Copy link
Contributor

Thanks for your contribution!

If I read correctly, this.$matchIterator(session, options) could have a falsy return in which case this function will return false as well, so shouldn't the return type here be Range | null | false?

As a second note, could you also run npm run update-types to update the *.d.ts type declaration files.

@ferdnyc
Copy link
Contributor Author

ferdnyc commented Jan 12, 2025

@akoreman

If I read correctly, this.$matchIterator(session, options) could have a falsy return in which case this function will return false as well, so shouldn't the return type here be Range | null | false?

Eek, you're right. I missed the early return up top there. I'll fix that, thanks.

As a second note, could you also run npm run update-types to update the *.d.ts type declaration files.

Shall do.

@ferdnyc
Copy link
Contributor Author

ferdnyc commented Jan 12, 2025

@akoreman

Part of me now wonders, though, if the early return shouldn't just be,

if (!iterator)
    return null;

...to avoid having to deal with two return types.

Although I suppose find() returning false rather than null (which happens iff $matchIterator() returns false, which in turn can only happen if $assembleRegExp() returns false) could be treated by the caller as an indication that something has gone wrong in the call, as opposed to a "no results" result.

@akoreman akoreman merged commit 85f01ca into ajaxorg:master Jan 13, 2025
@akoreman
Copy link
Contributor

It's probably best to leave the actual return signature as-is to prevent potential breaking changes, thanks for correcting the type declaration 🚀

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