-
Notifications
You must be signed in to change notification settings - Fork 4.8k
Add .gitattributes override mention when returning the strategy #7600
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
base: main
Are you sure you want to change the base?
Conversation
|
I think it would be clearer to users if we reported "(overridden by [path/to/.gitattributes])" instead of merely "GitAttributes" (as there's really only one that affects Linguist's classification). So instead of treating |
|
Sounds good, I've made the change to display the information like so:
This change means that detection has to take place even when there is an override in order to determine what strategy would be used. However, the changes in e879628 also introduce a check on Adding the |
lildude
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like a good idea to me. Can you please also update the README.md to document and show the output this change introduces.
In the latest commits, I've added:
|
README.md
Outdated
| … | ||
| ``` | ||
|
|
||
| If a file's language was overridden using `.gitattributes`, the strategy will show the original detection method along with an override note: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"strategy will show the original detection method along with an override note"
🤔 this is confusing with the output below. devcontainer.json is already JSONC but the output suggests it's overridden to JSONC by an override. Why would you override to the same language?
I think it would be better to use an example where there is a clear difference in languages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better to use an example where there is a clear difference in languages.
Ok, I'll change it for a better example... I'm thinking of using .bas file override in https://github.com/tannerhelland/PhotoDemon
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know why, but it's the only "override" in the linguist repo 🤷
Whoops. I'm not sure how that slipped past. We shouldn't need that override.
As an aside, we use the test/attributes branch for testing overrides. The .gitattributes in that branch has a lot more overrides.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This discussion makes me think that maybe the implementation of the --strategies flag should make the note appears different if the detection gives the same language as the override.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something like:
# Get the original strategy by calling super (which calls Linguist.detect)
original_language = super
original_strategy_info = Linguist.instrumenter.detected_info[self.name]
original_strategy = original_strategy_info ? original_strategy_info[:strategy] : "Unknown"
# Determine if gitattributes actually changed the result
if original_language == detected_language
strategy_name = "#{original_strategy} (confirmed by .gitattributes)"
else
strategy_name = "#{original_strategy} (overridden by .gitattributes)"
end
This pull request introduces
.gitattributesoverrides as the reported "strategy" when using the--strategiesoption. It also adds small integration tests to verify that CLI behavior reflects these changes while preserving the normal behavior when--strategiesis not specified.Language detection improvements:
linguist-languageattribute in.gitattributes, define the strategy asGitAttributes.Testing and instrumentation enhancements:
test_basic_instrumenter.rbto verify that the detection strategy and language are correctly tracked when.gitattributesoverrides are present, and that the strategy is recorded asGitAttributes.CLI integration and coverage:
test_cli_integration.rbsuite with tests for CLI flags (--strategies,--breakdown,--json) to ensure that.gitattributesoverrides are detected, the correct strategy is reported, and JSON output remains accurate.