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

Allow app/chewy location to be changed via yml config #414

Merged
merged 2 commits into from
Aug 13, 2016
Merged

Allow app/chewy location to be changed via yml config #414

merged 2 commits into from
Aug 13, 2016

Conversation

robacarp
Copy link

@robacarp robacarp commented Aug 8, 2016

Hello again!

In our rails application directory we have the following folders:

assets/
contexts/
controllers/
errors/
exporters/
helpers/
jobs/
mailers/
models/
policies/
serializers/
views/

While integrating the Chewy gem, I added the app/chewy folder to the application. There were a few complaints that 'chewy' didn't match the convention applied in the app/ directory, and so I was able to change it to app/indices with a little creative monkey patching.

This patch allows the directory to be set with a new config option called index_definition_path.

I tried to find every place the path was hard coded and replace with Chewy.configuration[:index_definition_path].

Additionally, the default setting is app/chewy so current deployments shouldn't need to change anything.

Thoughts?

Thanks!

@pyromaniac
Copy link
Contributor

The only this here that concerns me - is the config entry name. index_definition_path is not quite clear. I would think about it. Everything else is awesome.

@robacarp
Copy link
Author

robacarp commented Aug 8, 2016

@pyromaniac I agree, but I couldn't come up with anything that seemed more obvious. I'm open to suggestions if you have them.

@pyromaniac
Copy link
Contributor

indices_path or indexes_path

@pyromaniac
Copy link
Contributor

I'm not sure, which one, I can't choose :) Actually, we've got both usages all over the gem. Probably it makes sense to pick one, indices, probably. So let's use indices_path.

@robacarp
Copy link
Author

robacarp commented Aug 9, 2016

@pyromaniac excellent! Updated

@robacarp
Copy link
Author

Sorry, It looks like I fat fingered that entire commit.

@pyromaniac pyromaniac merged commit 2e4de96 into toptal:master Aug 13, 2016
@pyromaniac
Copy link
Contributor

Awesome, thanks!

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