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

Promote did_you_mean to default gem #2631

Closed
wants to merge 1 commit into from

Conversation

kddnewton
Copy link
Contributor

At the moment, there are some problems with regard to bundler + did_you_mean because of did_you_mean being a bundled gem. Since the vendored version of thor inside bundler and ruby itself explicitly requires did_you_mean, it can become difficult to load it when using Bundler.setup. See this issue: ruby/did_you_mean#117 (comment) for more details.

This is my first PR to Ruby so apologies if I didn't follow the directions correctly. As for the PR itself, I basically just updated tool/sync_default_gems.rb, made a couple of doc updates, and then ran the script for just did_you_mean.

@yuki24 feel free to open your own PR or whatever you need to do, this is just to get it started.

@yuki24
Copy link
Member

yuki24 commented Oct 29, 2019

Thanks for taking the time to send this PR! I will look at it more closely later. I might have to replace minitest with test-unit but that shouldn’t be too difficult. I’ll also take a look at the test failures to see if I can fix them.

@yuki24 yuki24 self-assigned this Oct 29, 2019
@yuki24
Copy link
Member

yuki24 commented Oct 30, 2019

The repo has been moved to ruby/did_you_mean. The next step would be to replace minitest with test-unit. I will try to do that this weekend.

@kddnewton
Copy link
Contributor Author

Nice - I've updated this PR to reflect the new location

@kddnewton
Copy link
Contributor Author

Some tests failing that I think ruby/did_you_mean#131 will fix.

At the moment, there are some problems with regard to bundler + did_you_mean because of did_you_mean being a bundled gem. Since the vendored version of thor inside bundler and ruby itself explicitly requires did_you_mean, it can become difficult to load it when using Bundler.setup. See this issue: ruby/did_you_mean#117 (comment) for more details.
@kddnewton
Copy link
Contributor Author

Okay good it's to the point of just breaking other tests now, which can probably be relaxed. Looks like they're all with TracePoint and throwing exceptions and matching on errors. As in:

https://github.com/ruby/ruby/commit/20a8cd310dfe847cff5c50f184626be46b1c7599/checks#step:10:153

@yuki24
Copy link
Member

yuki24 commented Nov 18, 2019

Alright, I have removed experimental features that depended on TracePoint. I don't think it is worth maintaining and it's more important to bring it in. Let's see how it goes.

rm_rf(%w[lib/did_you_mean* test/did_you_mean])
cp_r(Dir.glob("#{upstream}/lib/did_you_mean*"), "lib")
cp_r("#{upstream}/did_you_mean.gemspec", "lib/did_you_mean")
cp_r("#{upstream}/test", "test/did_you_mean")
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to remove this file test/did_you_mean/tree_spell/test_explore.rb? I know it's awkward, but this test particularly takes more than 5min and is not written in a normal testing fashion. I'm looking to improve it but haven't had time to do so. I would like to exclude it until we come up with a better solution.

Suggested change
cp_r("#{upstream}/test", "test/did_you_mean")
cp_r("#{upstream}/test", "test/did_you_mean")
rm_rf(%w[test/did_you_mean/tree_spell/test_explore.rb])

@yuki24
Copy link
Member

yuki24 commented Dec 1, 2019

closing in favor of #2689.

@yuki24 yuki24 closed this Dec 1, 2019
@kddnewton kddnewton deleted the did-you-mean-default branch December 2, 2019 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants