Skip to content

[#3808] flag ppport.h files created by Devel::PPPort perl module as generated#4106

Merged
lildude merged 2 commits intogithub-linguist:masterfrom
ioanrogers:perl-generated-ppport.h
May 2, 2018
Merged

[#3808] flag ppport.h files created by Devel::PPPort perl module as generated#4106
lildude merged 2 commits intogithub-linguist:masterfrom
ioanrogers:perl-generated-ppport.h

Conversation

@ioanrogers
Copy link
Contributor

Marking ppport.h files which are generated by Devel::PPPort as generated.

Checklist:

  • I am adding new or changing current functionality
    • I have added or updated the tests for the new or changed functionality.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why 10? Is that arbitrary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The string match in the next test is on line 9, so it's just to make sure the file is at least that long.

Copy link
Contributor

@pchaigno pchaigno Apr 20, 2018

Choose a reason for hiding this comment

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

Ah, ok. I guess I was expecting >= 9 or a higher number for the whole header comment.

Copy link
Member

Choose a reason for hiding this comment

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

Will this always be on line 9?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

I think in that case we can probably skip the lines.count > 10 check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In Perl, if you try to perform a match against something which is undefined you will get a warning.
I don't know Ruby - does the include call handle array elements which don't exist?

Copy link
Contributor Author

@ioanrogers ioanrogers Apr 19, 2018

Choose a reason for hiding this comment

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

Okay, I just checked with a truncated file and it seemed ok.

I forgot to take out the check before running the test on a truncated file. It does indeed give a warning:

TestGenerated#test_check_generated:
NoMethodError: undefined method `include?' for nil:NilClass
    /home/ioanrogers/Projects/PUBLIC_GITHUB/linguist/lib/linguist/generated.rb:543:in `generated_perl_ppport_header?'
    /home/ioanrogers/Projects/PUBLIC_GITHUB/linguist/lib/linguist/generated.rb:89:in `generated?'
    /home/ioanrogers/Projects/PUBLIC_GITHUB/linguist/lib/linguist/generated.rb:12:in `generated?'
    /home/ioanrogers/Projects/PUBLIC_GITHUB/linguist/test/test_generated.rb:20:in `generated_loading_data'
    /home/ioanrogers/Projects/PUBLIC_GITHUB/linguist/test/test_generated.rb:28:in `generated_fixture_loading_data'
    /home/ioanrogers/Projects/PUBLIC_GITHUB/linguist/test/test_generated.rb:123:in `test_check_generated'

Copy link
Member

Choose a reason for hiding this comment

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

Ah, good point. 👍

@lildude
Copy link
Member

lildude commented Apr 20, 2018

This is looking good from my perspective, except for the heeeaaawwg ppport.h file. As this is only being used for testing, I think we can safely truncate it to about 30 lines with a comment at the point of truncation along the lines of "linguist: deliberately truncated for test purposes" or something along those lines.

Thoughts?

@ioanrogers
Copy link
Contributor Author

Done

Copy link
Member

@lildude lildude left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the PR and welcome to Linguist.

@lildude lildude merged commit 59f5b05 into github-linguist:master May 2, 2018
lildude added a commit that referenced this pull request May 30, 2018
@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.

3 participants