fix(heartbeat): add language fallback detection from file extension#5
fix(heartbeat): add language fallback detection from file extension#5
Conversation
When vim.bo[bufnr].filetype is empty, detect language from file extension. This ensures the language field is populated even when Neovim's filetype detection doesn't recognize the file. - Add lua/shelltime/utils/language.lua with extension mapping - Update heartbeat.lua to use lang.get_language() - Add comprehensive tests for language detection 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Summary of ChangesHello @AnnatarHe, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Code Review: Language Fallback DetectionThanks for this contribution! Overall, this is a solid implementation that addresses the need for fallback language detection. Here's my detailed review: ✅ Strengths
🐛 Potential Issues1. Extension Mapping Inconsistency (lua/shelltime/utils/language.lua:23-24)cc = 'cpp',
cxx = 'cpp',
h = 'c',
hpp = 'cpp',The
2. Case-Sensitivity in Special Filename Detection (lua/shelltime/utils/language.lua:67-72)local filename = file_path:match('[/\\]?([^/\\]+)$') or ''
local basename = filename:lower()
if basename == 'dockerfile' thenThe regex pattern local filename = file_path:match('[^/\\]+$') or ''3. Missing Common LanguagesThe extension map is missing some common file types:
Not blocking, but worth considering for completeness. ⚡ Performance Considerations
🔒 Security ConcernsNo security issues identified. The module:
📝 Code Quality Suggestions1. Simplify Extension Detection (lua/shelltime/utils/language.lua:59-63)Current code: local ext = file_path:match('%.([^%.]+)$')
if ext then
ext = ext:lower()
return extension_map[ext] or ext
endCould be more defensive: local ext = file_path:match('%.([^%.]+)$')
if not ext or ext == '' then
-- Check special filenames before returning empty
goto check_special
end
ext = ext:lower()
return extension_map[ext] or ext
::check_special::
-- Special case handling...But current approach is fine - the special filename check handles this case. 2. Consider Adding Windows Path SupportThe special filename detection uses 🧪 Test CoverageExcellent test coverage! The tests cover:
Suggestion: Add a test for Windows-style paths with backslashes: it('should handle Windows paths', function()
assert.equals('lua', language.get_language('', 'C:\\path\\to\\file.lua'))
end)📚 DocumentationThe LuaDoc annotations are good: ---@param filetype string|nil Buffer filetype
---@param file_path string File path
---@return string Language identifierConsider adding a module-level comment explaining the fallback strategy: --- Language detection utilities for shelltime
--- Provides fallback language detection from file extension when filetype is empty
---
--- Detection order:
--- 1. Use vim filetype if available (highest priority)
--- 2. Detect from file extension (case-insensitive)
--- 3. Check for special filenames (Dockerfile, Makefile)
--- 4. Return unknown extension as-is or empty string🎯 Integration PointsThe integration in language = lang.get_language(vim.bo[bufnr].filetype, file_path),✅ Passes both filetype and file_path 📋 RecommendationsBefore merging:
Nice to have (future PRs):
📊 Overall AssessmentQuality: ⭐⭐⭐⭐ (4/5) This is a well-crafted PR that solves a real problem with minimal risk. The code follows project conventions, has excellent test coverage, and doesn't introduce breaking changes. The issues I've noted are minor and mostly suggestions for enhancement rather than critical fixes. Recommendation: ✅ Approve with minor suggestions Great work! 🎉 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively adds a fallback mechanism for language detection using file extensions, which is a great enhancement. The new language.lua utility is well-structured and comes with a comprehensive set of tests. I have a couple of suggestions to improve the maintainability of the new utility and its tests, primarily by using table-driven logic to reduce repetition and make future extensions easier.
| -- Special case: Dockerfile, Makefile, etc. | ||
| local filename = file_path:match('[/\\]?([^/\\]+)$') or '' | ||
| local basename = filename:lower() | ||
| if basename == 'dockerfile' then | ||
| return 'dockerfile' | ||
| elseif basename == 'makefile' then | ||
| return 'makefile' | ||
| end | ||
|
|
||
| return '' |
There was a problem hiding this comment.
The special filename handling can be made more maintainable and scalable by using a lookup table instead of an if/elseif chain. This makes it easier to add more special filenames in the future.
For better performance, you could also consider moving the special_filenames table outside the get_language function to avoid re-creating it on every call.
-- Special case: Dockerfile, Makefile, etc.
local special_filenames = {
dockerfile = 'dockerfile',
makefile = 'makefile',
}
local filename = file_path:match('([^/\\]+)$')
if filename then
local lang = special_filenames[filename:lower()]
if lang then
return lang
end
end
return ''
| describe('when filetype is available', function() | ||
| it('should return filetype as-is for lua', function() | ||
| assert.equals('lua', language.get_language('lua', '/path/to/file.lua')) | ||
| end) | ||
|
|
||
| it('should return filetype as-is for python', function() | ||
| assert.equals('python', language.get_language('python', '/path/to/file.py')) | ||
| end) | ||
|
|
||
| it('should return filetype as-is for typescript', function() | ||
| assert.equals('typescript', language.get_language('typescript', '/path/to/file.ts')) | ||
| end) | ||
|
|
||
| it('should return filetype as-is for javascript', function() | ||
| assert.equals('javascript', language.get_language('javascript', '/path/to/file.js')) | ||
| end) | ||
|
|
||
| it('should return filetype as-is for sh', function() | ||
| assert.equals('sh', language.get_language('sh', '/path/to/file.sh')) | ||
| end) | ||
|
|
||
| it('should return filetype as-is for vim', function() | ||
| assert.equals('vim', language.get_language('vim', '/path/to/file.vim')) | ||
| end) | ||
|
|
||
| it('should return filetype even if extension differs', function() | ||
| assert.equals('python', language.get_language('python', '/path/to/file.txt')) | ||
| end) | ||
| end) |
There was a problem hiding this comment.
This block of tests is quite repetitive. You can use a data-driven approach to make the tests more concise and easier to maintain. This involves defining a table of test cases and iterating over it to create the tests.
This pattern can also be applied to other repetitive test blocks in this file, such as 'when filetype is empty' and 'special filenames'.
describe('when filetype is available', function()
local test_cases = {
{ name = 'lua', filetype = 'lua', path = '/path/to/file.lua' },
{ name = 'python', filetype = 'python', path = '/path/to/file.py' },
{ name = 'typescript', filetype = 'typescript', path = '/path/to/file.ts' },
{ name = 'javascript', filetype = 'javascript', path = '/path/to/file.js' },
{ name = 'sh', filetype = 'sh', path = '/path/to/file.sh' },
{ name = 'vim', filetype = 'vim', path = '/path/to/file.vim' },
{ name = 'python with different extension', filetype = 'python', path = '/path/to/file.txt' },
}
for _, tc in ipairs(test_cases) do
it('should return filetype as-is for ' .. tc.name, function()
assert.equals(tc.filetype, language.get_language(tc.filetype, tc.path))
end)
end
end)
Files like .hidden, .gitignore should return empty string, not the filename after the dot. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Pull Request Review: Language Fallback DetectionThanks for this contribution! This PR adds a useful feature to handle cases where Neovim's filetype detection fails. Here's my detailed review: ✅ Strengths
🔍 Potential Issues1. Pattern Matching Edge Case (Minor)Line local filename = file_path:match('[/\\]?([^/\\]+)$') or ''The pattern
Suggestion: Simplify to 2. Unknown Extension Behavior (Design Question)Line return extension_map[ext] or extWhen an unknown extension is encountered (e.g., Questions:
3. Missing Integration Test (Minor)The new Suggestion: Add a test in 📊 Performance Considerations
Overall: No performance issues expected. 🔒 Security ConcernsNo security issues identified:
📝 Minor Suggestions
🎯 Test Plan VerificationThe PR description mentions a test plan but the checkboxes are empty. Before merging:
🏁 VerdictApprove with minor suggestions This is a solid PR that solves a real problem with good test coverage and clean implementation. The suggested improvements are minor and optional. The code is ready to merge as-is, though addressing the edge cases mentioned would make it even more robust. Great work! 🎉 |
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. ℹ️ You can also turn on project coverage checks and project coverage reporting on Pull Request comment Thanks for integrating Codecov - We've got you covered ☂️ |
1 participant
Summary
vim.bo[bufnr].filetypeis emptylua/shelltime/utils/language.luawith extension-to-language mappingheartbeat.luato uselang.get_language(filetype, file_path)Test plan
./scripts/test.shto verify all tests pass🤖 Generated with Claude Code