Skip to content

Conversation

@palandovalex
Copy link

#32

solved

return get_local_env(venv_path)
end
if not file then
if isdir(venv_path) then
Copy link
Owner

Choose a reason for hiding this comment

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

if isdir(venv_path) is true shouldn't the above line already have returned so no need for this statement here?

local file = io.open(project_dir .. '/.venv', 'r') -- r read mode
M.get_project_venv_data = function(project_dir)
local venv_path = project_dir .. '/.venv'
local file = io.open(venv_path, 'r') -- r read mode
Copy link
Owner

Choose a reason for hiding this comment

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

maybe move this below the if-statement since it's not needed if the body is executed

-- Get the name from a `.venv` file in the project root directory.
M.read_venv_name = function(project_dir)
local file = io.open(project_dir .. '/.venv', 'r') -- r read mode
M.get_project_venv_data = function(project_dir)
Copy link
Owner

Choose a reason for hiding this comment

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

this function is now a bit strange, it either returns a string or a table? Would be better to have a single return type

local closest_match = best_match(venvs, project_venv_name)
if type(venv_data) == type({}) then
vim.list_extend(venvs, venv_data)
set_venv(venv_data)
Copy link
Owner

Choose a reason for hiding this comment

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

would be nicer if this and the below could be unified, see my comment about the return type of get_project_venv_data

@palandovalex palandovalex force-pushed the support_local_venv_dirs branch from e689014 to 5f66f35 Compare March 13, 2024 09:22
@AckslD
Copy link
Owner

AckslD commented Mar 22, 2024

Hi @palandovalex, let me know when you want me to take another look. Just fyi I will assume you're still working on the PR until you reply to the comments :)

@Nicola-Bini
Copy link

@palandovalex I am also interested in this. Let me know if you need support to finalize the PR by addressing @AckslD comments. I can spend sometime on this in the next 1-2 weeks.

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