Skip to content

Conversation

@scarbajali
Copy link
Contributor

@scarbajali scarbajali commented Jul 25, 2025

I usually work with preinstalled versions of vim which doesn't have +python3 compiled and I see that in this case the plugin is fully disabled.

This PR will switch to the python version installed in the system in case embedded python is not found and only OllamaEdit will be disabled (error messages are displayed if the user tries to use it).

Copy link
Contributor

@ilya-bobyr ilya-bobyr left a comment

Choose a reason for hiding this comment

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

A few notes.

Copy link
Contributor

@ilya-bobyr ilya-bobyr left a comment

Choose a reason for hiding this comment

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

Looks good to me.
A few minor notes.

Comment on lines +17 to +18
if !ollama#edit#HasEmbeddedPython()
echoerr "OllamaEdit features require Vim compiled with +python3 support."
Copy link
Contributor

Choose a reason for hiding this comment

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

Considering the same error message is repeated after every invocation to ollama#edit#HasEmbeddedPython() maybe it would make sense to move the error message into the same function?

function! ollama#edit#EnsureEmbeddedPythonIsAvailable() abort
    if exists('g:ollama_embedded_python') && g:ollama_embedded_python != 0
        echoerr "OllamaEdit features require Vim compiled with +python3 support."
        return v:true
    else
        return v:false
    endif
endfunction

And then the usage looks like this:

    if !ollama#edit#EnsureEmbeddedPythonIsAvailable()
        return
    endif


" Function to check if embedded Python is available
function! ollama#edit#HasEmbeddedPython() abort
return exists('g:ollama_embedded_python') && g:ollama_embedded_python != 0
Copy link
Contributor

Choose a reason for hiding this comment

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

minor

In autoload/ollama/setup.vim only the value of g:ollama_embedded_python is checked:

            if g:ollama_embedded_python
              call s:SetupPyVEnv()
            endif

Here both existence and value are checked. While checking existence is more robust, if the assumption is that this value will always be set by the plugin setup code, the existence check becomes redundant.

In any case, it seems inconsistent to treat this variable one way in one location and another way in another.
I think either both locations should check existence, or neither should.
I would probably recommend the latter.
It would make the check simpler.

Comment on lines +427 to +431
venv_site_packages = os.path.join(
venv_path,
'lib', f'python{sys.version_info.major}.{sys.version_info.minor}',
'site-packages'
)
Copy link
Contributor

Choose a reason for hiding this comment

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

minor

If you are switching to vertical formatting, maybe format all the argument vertically:

Suggested change
venv_site_packages = os.path.join(
venv_path,
'lib', f'python{sys.version_info.major}.{sys.version_info.minor}',
'site-packages'
)
venv_site_packages = os.path.join(
venv_path,
'lib',
f'python{sys.version_info.major}.{sys.version_info.minor}',
'site-packages'
)

sys.path.insert(0, venv_site_packages)
else:
print('Venv not found: '. venv_path)
print('Venv not found: ' + venv_path)
Copy link
Contributor

Choose a reason for hiding this comment

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

minor

This has been fixed already.
It seems that this project uses merges for PRs, so maybe it does not matter much.
But if you rebase this minor fix should disappear.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants