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

Add some linguist-detectable attributes #3806

Merged
merged 1 commit into from Sep 19, 2017

Conversation

@seppestas
Copy link
Contributor

commented Aug 31, 2017

This allows to test a use-case where markdown should be reported
in the language stats (e.g because the main purpose of the repo is to
hold prose in markdown format) while HTML should be excluded from the
language stats (e.g because it's example output).

Add some linguist-detectable attributes
This allows to test a use-case where markdown should be reported
in the language stats (e.g because the main purpose of the repo is to
hold prose in markdown format) while HTML should be excluded from the
language stats (e.g because it's example output).
@seppestas

This comment has been minimized.

Copy link
Contributor Author

commented Aug 31, 2017

This is required by the tests added in #3807.

@lildude

This comment has been minimized.

Copy link
Member

commented Sep 7, 2017

My comment at #3807 (comment) applies here too.

@lildude

This comment has been minimized.

Copy link
Member

commented Sep 9, 2017

As this PR makes no sense on its own and is only needed for the functionality you're proposing in #3807, can you please incorporate the changes into #3807 and close this PR.

@pchaigno

This comment has been minimized.

Copy link
Collaborator

commented Sep 9, 2017

@lildude The 2 pull requests are based on different branches. We have a special test/attributes branch for some of the tests.

@lildude

This comment has been minimized.

Copy link
Member

commented Sep 9, 2017

Oh yes. I completely missed that. 😊 Ignore my comment @seppestas

@seppestas

This comment has been minimized.

Copy link
Contributor Author

commented Sep 9, 2017

Any chance this could get changed by the way?

It took me a bit to figure out how testing Linguist gitattributes work and it looks like even some regular contributors are not aware of how it works.

IMO it would make sense to have a seperate test repository on which to run tests instead of using Linguist's own repo. This allows to test gitattributes without the need for a test/gitattributes branch, and it would make a lot of other test relying on git a lot faster, since Linguist's history is pretty big.

The test repository could be added to the main repo as a submodule (possibly even multiple times), allowing to update the submodule in PR's to the main repo when e.g changes to the gitattributes are needed for new tests.

@lildude

This comment has been minimized.

Copy link
Member

commented Sep 19, 2017

Any chance this could get changed by the way?

Not at the moment. I agree it's a bit odd and I didn't know about it until now (I've not touched that side of the code) and it makes sense, however I don't think this and the repo size is enough substantiation to pull the test data away from the code actually being tested at this point primarily as it would add more complexity to contributing to Linguist: we have way more contributions that don't affect git attributes than we do that do. Maybe in future, until then I think improving CONTRIBUTING.md to highlight this is better in the interim.

Before approving this, why have you chosen to test with markdown and HTML rather than KiCad, the language motivating all these changes?

Just wondering if it makes more sense to test the override for KiCad to keep the change in context with the changes in #3807.

@seppestas

This comment has been minimized.

Copy link
Contributor Author

commented Sep 19, 2017

Before approving this, why have you chosen to test with markdown and HTML rather than KiCad, the language motivating all these changes?

Because my implementation was built on top of the changes that make KiCad files detectable by default. While I plan to remove that feature from #3807, I still think files like KiCad design files should be included in the language stats by default.

I mainly added the linguist-detectable field to address concerns like

it could open up a common vector for disputes (i.e. some folks want XML files to be detectable by default and some don't)
-- @rafer in #3807

Markdown and HTML are very commonly known, and I think the use-case I described makes sense for e.g repo's containing some sort of website like a blog. XML files in Android projects would be another great use-case, but I'm not that familiar with that.

As a sidenote: one of the reasons KiCAD motivated these changes is because it used to be included in language stats, but since #3743 this stopped working. For some reason Github still includes Eagle files in the language stats, even though Linguist stopped reporting them since #3751 (according to @Alhadis this is probably caused by some sort of caching). Eagle is still a lot more popular than KiCAD, so if it wasn't for this glitch I guess Eagle would have been the motivation 😅.

@Alhadis

This comment has been minimized.

Copy link
Collaborator

commented Sep 19, 2017

Just out of modest curiosity, what exactly distinguishes Eagle from any other XML-derived language? Every example I've seen is essentially just XML data, and probably belongs in the ever-growing morass of vendor-specific XML files.

I can understand if you really don't want to answer this question. 😜

@seppestas

This comment has been minimized.

Copy link
Contributor Author

commented Sep 19, 2017

As for changing the way Linguist is tested: I think having separate test repositories would reduce complexity and increase reproducibility.

Also, AFAIK most contributions to Linguist don't add that much test code. Typically contributions just add a language and some test files for the language.

Because of the slow Ruby VM and the big git repo history, running all the test of Linguist takes a lot of time, making a task that should be done often pretty tedious.This is coming from someone used to building huge C++ projects.

@seppestas

This comment has been minimized.

Copy link
Contributor Author

commented Sep 19, 2017

Just out of modest curiosity, what exactly distinguishes Eagle from any other XML-derived language? Every example I've seen is essentially just XML data, and probably belongs in the ever-growing morass of vendor-specific XML files.

I believe Eagle is indeed just vendor-specific XML (but I'm not that familiar with the Eagle format). That being said, I would very much like to keep having it included in the language stats. For the purpose of syntax highlighting I guess calling it XML would work fine.

@Alhadis

This comment has been minimized.

Copy link
Collaborator

commented Sep 19, 2017

I would very much like to keep having it included in the language stats.

Authors of TextMate themes, WiX websites, and X3D models would probably prefer that degree of recognition too.

Not to worry, I don't recall a language ever being removed in Linguist's history... although I could be wrong.

@lildude
Copy link
Member

left a comment

Keeping things solely within discussion surrounding the motivation for these specific examples, your explanation makes sense and has satisfied my curiosity. Thanks.

Please continue discussions around everything else not directly related in independent issues.

@lildude lildude merged commit 8f86998 into github:test/attributes Sep 19, 2017

2 checks passed

GitHub CLA @seppestas has accepted the GitHub Contributor License Agreement.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

seppestas added a commit to seppestas/linguist that referenced this pull request Sep 19, 2017

Update commit hash to the one that was merged
PR github#3806 changed the commit hash. The original commit was not
actually merged into the test/attributes branch.

seppestas added a commit to seppestas/linguist that referenced this pull request Jan 12, 2018

Update commit hash to the one that was merged
PR github#3806 changed the commit hash. The original commit was not
actually merged into the test/attributes branch.

lildude added a commit that referenced this pull request Jan 23, 2018

Override languages being included by language statistics (#3807)
* Add detectable key to languages

This key allows to override the language being included in the
language stats of a repository.

* Make detectable override-able using .gitattributes

* Mention `linguist-detectable` in README

* Remove detectable key from languages

Reverts changes in 0f7c0df.

* Update commit hash to the one that was merged

PR #3806 changed the commit hash. The original commit was not
actually merged into the test/attributes branch.

* Fix check to ensure detectable is defined

* Add include in language stats tests when detectable set

* Ignore detectable when vendored, documentation or overridden

* Add documentation on detectable override in README

* Improve documentation on detectable override in README
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.