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

Ensure generated_html? handles single line files #4620

Merged
merged 1 commit into from
Aug 23, 2019

Conversation

lildude
Copy link
Member

@lildude lildude commented Aug 23, 2019

Description

Whilst testing for the next release of Linguist, I found one of the GitHub.com tests failed as follows:

bundler: failed to load command: bin/github-linguist (bin/github-linguist)
NoMethodError: undefined method `start_with?' for nil:NilClass
  /Users/lildude/github/linguist/lib/linguist/generated.rb:612:in `generated_html?'
  /Users/lildude/github/linguist/lib/linguist/generated.rb:89:in `generated?'
  /Users/lildude/github/linguist/lib/linguist/generated.rb:12:in `generated?'
  /Users/lildude/github/linguist/lib/linguist/blob_helper.rb:357:in `generated?'
  /Users/lildude/github/linguist/lib/linguist/lazy_blob.rb:52:in `generated?'
  /Users/lildude/github/linguist/lib/linguist/blob_helper.rb:380:in `include_in_language_stats?'
  /Users/lildude/github/linguist/lib/linguist/repository.rb:164:in `block in compute_stats'
  /Users/lildude/github/linguist/lib/linguist/repository.rb:149:in `each_delta'
  /Users/lildude/github/linguist/lib/linguist/repository.rb:149:in `compute_stats'
  /Users/lildude/github/linguist/lib/linguist/repository.rb:116:in `cache'
  /Users/lildude/github/linguist/lib/linguist/repository.rb:68:in `languages'
  bin/github-linguist:30:in `<top (required)>'

The test is failing on a very simple .html file that contains:

I'm remote.html, a different file loaded with ajax.

The problem is the generated_html? method makes a few assumptions about the number of lines in the file and doesn't consider there may only be a single line.

This PR corrects that behaviour .

Checklist doesn't apply as I'm fixing a bug and we don't have a section for that 😄.

Copy link
Contributor

@pchaigno pchaigno left a comment

Choose a reason for hiding this comment

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

LGTM!

We should have a test for this, but I don't see an easy way to write one.

@lildude
Copy link
Member Author

lildude commented Aug 23, 2019

Yeah, I couldn't find an easy way to test this either.

@lildude lildude merged commit 701ae4f into master Aug 23, 2019
@lildude lildude deleted the lildude/fix-gen-html branch August 23, 2019 14:29
@Alhadis
Copy link
Collaborator

Alhadis commented Aug 29, 2019

Oops, sorry about that. Chalk that one up to my lack of Ruby experience. 😅

@lildude Is there anything I can do to help with whatever's going on at lildude/fix-gen_html-error...? Or is it unrelated to my earlier PR's changes?

Also, major WTF @ the recent commit logs on your branch:

Author: The rugged tests are fragile <colin@github.com>

/cc @TheRuggedFragileTestFormerlyKnownAsLilDude

@lildude
Copy link
Member Author

lildude commented Aug 29, 2019

@lildude Is there anything I can do to help with whatever's going on at lildude/fix-gen_html-error...? Or is it unrelated to my earlier PR's changes?

Just opened a PR for that - #4628

Also, major WTF @ the recent commit logs on your branch:

Author: The rugged tests are fragile <colin@github.com>

Whoops. Not sure how I did that 😊 . Will rewrite my author info.

@github-linguist github-linguist locked as resolved and limited conversation to collaborators Jun 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants