5 ms·

Inciminated comment: https://github.com/github/linguist/pull/748#issuecomment-37418098 https://github.com/github/linguist/pull/748#issuecomment-374... > if you

by bru 13y ago

Inciminated comment: https://github.com/github/linguist/pull/748#issuecomment-37418098 https://github.com/github/linguist/pull/748#issuecomment-374... > if you'd like Mercury language detection on GitHub then with the current implementation of Linguist you need to pick a different (unique as Objective-C already defines this) primary_extension and add .m to the extensions array which will force Linguist into using the other detection methods mentioned above.

moron4hire 13y ago
what, then, is the point of the primary_extension field? EDIT: or as I like to yell at Github for Windows when it can't revert out of a merge conflict "WHAT IS EVEN THE POINT OF YOU?!"
- RyanZAG 13y ago
  Probably if the classifier can't determine the language, it will fall back to the primary_extension field - as it should, if the classifier can't determine if a .m file is objc or mercury, it should and will default to objc. Classification is never 100% accurate. EDIT: Exact method that it is used is reported here: https://github.com/github/linguist/pull/748#issuecomment-37418098 https://github.com/github/linguist/pull/748#issuecomment-374...
  - moron4hire 13y ago
    It's the other way around: the extension check is ran before the classifier. And given the limitations of file extensions, an extension collision is so likely of an occurrence (not most likely, but likely enough) as to not bother with primary_extension and just use the collection of extensions as a culling system to minimize the work for classification. In other words, it seems like the overall design wouldn't be hurt too much by just extricating primary_extension completely. Best-case scenario, primary_extension is equivalent to always having at least one item in extensions. It does nothing else. Also, this looks like a bug: "if possible_languages.length > 1 ... else possible_languages.first" What if length is 0? That's not greater than 1. I'm not familiar with Ruby, does first return null on an empty array, or does it error? LINQ in .NET has separate First<T> and FirstOrDefault<T> methods: one errors, the other returns default(T) (which is null in the case of reference types). Or is there a default match in the index that occurs when no other language is found? https://github.com/github/linguist/blob/master/lib/linguist/language.rb#L141-L144 https://github.com/github/linguist/blob/master/lib/linguist/... Not instilling a lot of confidence that someone really thought through this bit of code. I'm not saying I very strictly think through everything I write, but I also don't write software for thousands of users, and I acknowledge that I've grown rather complacent in terms of time spent per unit code.
    - moron4hire 13y ago
      I'm looking at more of this, and jesus christ, this is completely wrong: https://github.com/github/linguist/blob/master/lib/linguist/languages.yml#L39-L54 https://github.com/github/linguist/blob/master/lib/linguist/... Code that commonly resides in .asp files is completely different from code that commonly resides in .aspx files. They are not synonyms for each other. Also, I would wager that C# aspx files are a tad more common than VB.NET aspx files. It's even worse than lumping together .c and .cpp. You at least have some chance of getting .c files to compile in a C++ compiler. There is no chance of running ASP code through the ASP.NET engine. This is why "ASPX" as a term exists, to differentiate from ASP.
- skywhopper 13y ago
  It's just a design error in the original implementation where linguist assumes that the "primary_extension" for any particular language will be unique among all primary_extensions. Obviously that was a mistake, but that's where we are. The comment that set people off was perhaps poorly worded, but it was an honest suggestion to work around the design bug.
  - moron4hire 13y ago
    A better suggestion: fix the defect. Just delete primary_extension. At best, it does nothing that the extensions array can't do, as it doesn't appear at first glance that any check to primary_extension does not also include a check to extensions. At worst, it is confusing to implementers and requires chicanery to work around... which is exactly the case we're in. We're in the worst case scenario for this bit of code, and there is no upside to its best-case scenario. Just delete the code.
    - DangerousPie 13y ago
      I bet it's not as easy as "just delete the code". They will probably have to do quite a bit of refactoring to remove this, followed by a probably even larger amount of testing. Long-term this is probably the right solution, but why go through all this trouble right now if there is a simple workaround? It seems like the only problem right now is a few people's pride.
      - moron4hire 13y ago
        Of course it's not that easy. They don't have a compiler with a static checker to show them all the places the field was used :P
      - Allan_Smithee 13y ago
        How much you wanna bet? (https://github.com/github/linguist/issues/985 https://github.com/github/linguist/issues/985)
        13y ago

by bru 13y ago

moron4hire 13y ago

what, then, is the point of the primary_extension field? EDIT: or as I like to yell at Github for Windows when it can't revert out of a merge conflict "WHAT IS EVEN THE POINT OF YOU?!"

RyanZAG 13y ago
Probably if the classifier can't determine the language, it will fall back to the primary_extension field - as it should, if the classifier can't determine if a .m file is objc or mercury, it should and will default to objc. Classification is never 100% accurate. EDIT: Exact method that it is used is reported here: https://github.com/github/linguist/pull/748#issuecomment-37418098 https://github.com/github/linguist/pull/748#issuecomment-374...
- moron4hire 13y ago
  It's the other way around: the extension check is ran before the classifier. And given the limitations of file extensions, an extension collision is so likely of an occurrence (not most likely, but likely enough) as to not bother with primary_extension and just use the collection of extensions as a culling system to minimize the work for classification. In other words, it seems like the overall design wouldn't be hurt too much by just extricating primary_extension completely. Best-case scenario, primary_extension is equivalent to always having at least one item in extensions. It does nothing else. Also, this looks like a bug: "if possible_languages.length > 1 ... else possible_languages.first" What if length is 0? That's not greater than 1. I'm not familiar with Ruby, does first return null on an empty array, or does it error? LINQ in .NET has separate First<T> and FirstOrDefault<T> methods: one errors, the other returns default(T) (which is null in the case of reference types). Or is there a default match in the index that occurs when no other language is found? https://github.com/github/linguist/blob/master/lib/linguist/language.rb#L141-L144 https://github.com/github/linguist/blob/master/lib/linguist/... Not instilling a lot of confidence that someone really thought through this bit of code. I'm not saying I very strictly think through everything I write, but I also don't write software for thousands of users, and I acknowledge that I've grown rather complacent in terms of time spent per unit code.
  - moron4hire 13y ago
    I'm looking at more of this, and jesus christ, this is completely wrong: https://github.com/github/linguist/blob/master/lib/linguist/languages.yml#L39-L54 https://github.com/github/linguist/blob/master/lib/linguist/... Code that commonly resides in .asp files is completely different from code that commonly resides in .aspx files. They are not synonyms for each other. Also, I would wager that C# aspx files are a tad more common than VB.NET aspx files. It's even worse than lumping together .c and .cpp. You at least have some chance of getting .c files to compile in a C++ compiler. There is no chance of running ASP code through the ASP.NET engine. This is why "ASPX" as a term exists, to differentiate from ASP.
skywhopper 13y ago
It's just a design error in the original implementation where linguist assumes that the "primary_extension" for any particular language will be unique among all primary_extensions. Obviously that was a mistake, but that's where we are. The comment that set people off was perhaps poorly worded, but it was an honest suggestion to work around the design bug.
- moron4hire 13y ago
  A better suggestion: fix the defect. Just delete primary_extension. At best, it does nothing that the extensions array can't do, as it doesn't appear at first glance that any check to primary_extension does not also include a check to extensions. At worst, it is confusing to implementers and requires chicanery to work around... which is exactly the case we're in. We're in the worst case scenario for this bit of code, and there is no upside to its best-case scenario. Just delete the code.
  - DangerousPie 13y ago
    I bet it's not as easy as "just delete the code". They will probably have to do quite a bit of refactoring to remove this, followed by a probably even larger amount of testing. Long-term this is probably the right solution, but why go through all this trouble right now if there is a simple workaround? It seems like the only problem right now is a few people's pride.
    - moron4hire 13y ago
      Of course it's not that easy. They don't have a compiler with a static checker to show them all the places the field was used :P
    - Allan_Smithee 13y ago
      How much you wanna bet? (https://github.com/github/linguist/issues/985 https://github.com/github/linguist/issues/985)
      - 13y ago