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

Only rescue LoadError for linguist #160

Closed
wants to merge 1 commit into from

Conversation

calebhearth
Copy link

Currently this also catches a LoadError for rugged, a dependency Linguist
added but didn't add to its gemspec.

github-linguist/linguist#1600
github-linguist/linguist#1601

Currently this also catches a LoadError for rugged, a dependency
Linguist added but didn't add to its gemspec.

github-linguist/linguist#1600
github-linguist/linguist#1601
@jch
Copy link
Contributor

jch commented Oct 16, 2014

Thanks for the pull, but I'd rather see this fixed upstream in linguist rather than here.

@jch jch closed this Oct 16, 2014
@calebhearth
Copy link
Author

It's being fixed there, but you're also being potentially overzealous in your rescue.

@calebhearth
Copy link
Author

Or rather, there are PRs to fix it.

@jch
Copy link
Contributor

jch commented Oct 16, 2014

@calebthompson it sounds like rugged is a required dependency of linguist. With those fixes in place, why would there be a load error for rugged if the fixes are made upstream? Wouldn't attempting to load linguist fail if rugged wasn't installed?

I see that this would fix older versions of linguist, but I want to avoid more error handling for external dependencies. For 2.0, I have some ideas about defining adaptors for external dependencies so we can encapsulate these checks outside of the filter classes.

@calebhearth
Copy link
Author

Sorry, I just realized I left out important information here.

I found the problem in Linguist while trying to update Html Pipeline. I had trouble debugging it because of the rescue LoadError, and wouldn't have found the problem (upstream) at all if I hadn't added a require: to my project's Gemfile, because Html Pipeline was eating the LoadError (with the error message about 'rugged') and giving me a different message about not being having the github-linguist dependency.

So the bug this pull request solves is that our approach to having this gem's users handle dependencies also hides useful errors that don't result from not having its (direct) dependencies resolved.

@jch
Copy link
Contributor

jch commented Oct 16, 2014

Html Pipeline was eating the LoadError (with the error message about 'rugged') and giving me a different message about not being having the github-linguist dependency.

Ah, that is a good point. What do you think of updating the rescue message to output the raw error message then? Something along the lines of:

rescue LoadError => e
  abort "Missing dependency for SyntaxHighlightFilter. See README.md for details.\n#{e.message}"
end

If that sounds good, could you make the changes to the other filters as well?

@calebhearth
Copy link
Author

I'm fine with that, but I'll have to come back to the change.

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