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

Move the did_you_mean spellchecker to require-time #32792

Conversation

gsamokovarov
Copy link
Contributor

This change conditionally defines Rails::Command::Spellchecker based
on the did_you_mean presence in require time, so we don't have the
condition executing at runtime.

@rails-bot
Copy link

r? @schneems

(@rails-bot has picked a reviewer for you, use r? to override)

@utilum
Copy link
Contributor

utilum commented May 2, 2018

Smart :)

@kaspth
Copy link
Contributor

kaspth commented Jul 1, 2018

I find this harder to grok than the existing code. What's this costing us at runtime?

Copy link
Member

@sikachu sikachu left a comment

Choose a reason for hiding this comment

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

I seconded @kaspth comment. I wonder if we separate our own spellchecker and this file it'll make it much more cleaner (so this file would only do the conditional to see if did_you_mean exists or not, and if not it just call our new class).

@matthewd
Copy link
Member

matthewd commented Jul 2, 2018

I'm still dubious about the benefit of us using DYM here at all. When it meant we didn't have to maintain our own implementation (#32289), sure.. but now (#32781), I think any incremental gain we receive from some future DYM improvement is likely outweighed by the fact we're adapterizing something that just doesn't matter -- AFAIK we're not even testing both implementations?

@kaspth
Copy link
Contributor

kaspth commented Jul 2, 2018

@matthewd agreed, let's just kill it 👍

@gsamokovarov
Copy link
Contributor Author

Cool, will drop the whole did_you_mean thing.

@rails-bot
Copy link

rails-bot bot commented Dec 18, 2019

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

@rails-bot rails-bot bot added the stale label Dec 18, 2019
@gsamokovarov gsamokovarov force-pushed the check-for-did-you-mean-require-time branch from a697fc2 to 26335b3 Compare December 19, 2019 18:00
@rails-bot rails-bot bot removed the stale label Dec 19, 2019
@gsamokovarov gsamokovarov force-pushed the check-for-did-you-mean-require-time branch from 26335b3 to bd56be8 Compare December 19, 2019 18:37
@gsamokovarov
Copy link
Contributor Author

gsamokovarov commented Dec 19, 2019

The gentle bot nudge made me wanna finish this PR and remove the did_you_mean dependency, but I found out we have a feature that depends on the did_you_mean algorithm.

With the Levenstein algorithm, we always had suggestions as the first ones in the list are the closest ones to the given input. did_you_mean's spell checker is smart enough to give up and return no suggestions when they will be far from the truth. fb173b6

What do you folks think, is it worth it to remove the feature above and drop the did_you_mean implicit dependency or continue as we are now? I think dropping the constant nagging of Maybe you meant is worth the did_you_mean dependency, so I'm closing this PR now for now. Reopen if you think otherwise.

@kaspth
Copy link
Contributor

kaspth commented Feb 6, 2020

@gsamokovarov all good ✌️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants