Skip to content
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

bug fix: cyclic dependency causing sendline errors #382

Merged
merged 1 commit into from
Jul 9, 2024

Conversation

ottersome
Copy link
Contributor

Sending a line via send_line would prop up errors (see #379). Passing cmd via an anonymous function wrapper fixes it while avoiding the extra require call.

@nickeisenberg
Copy link
Contributor

nickeisenberg commented Jul 7, 2024

@ottersome I like your solution solution. I found a slightly different way to do this though that keeps bracketed_paste_python consistent with bracketed_paste. You can modify bracketed_paste_python in the following way:

common.bracketed_paste_python = function(lines)
  local result = {}

  local is_ipython = false
  if config.repl_definition.python ~= nil then
    if config.repl_definition.python.command ~= nil then
      is_ipython = contains(config.repl_definition.python.command, "ipython")
    end
  end

  -- rest is the same as before

What are your thoughts? It has been messy trying to 'python to work well with the repl. It seems that the fact that python does not have "enders" to if statements, functions, etc makes it hard to get the repl to work well.

edit:

Just realized mine has a flaw. If the user does not include a repl definition for python, the providers.lua will provide it and its possible that in that case, that the provider chooses ipython and in that case, my approach above fails. I will most likely merge yours into master. I will take a closer look later. Thanks again!

@ottersome
Copy link
Contributor Author

ottersome commented Jul 8, 2024

Hey @nickeisenberg , good to hear from you.
I guess my biggest fear with this is that the cycle is not really broken. I went through the commit history of iron.nvim and noticed that a dependency to config did not exist until it was needed for this specific function. I think introducing a require(config) might invite future changes to absentmindedly include functions from config without considering the reoccurance of this problem.

May I ask why the preference for consistency with bracketed_paste? The function seems to be very specific already. If it is for the sake of merging them again into a single function perhaps one can create a more general one where rather than passing the cmd paremeter in my commit on can pass a map of particularities that need to be taken care of on a per-language basis. These could overlap across languages after all.
e.g.

-- common.lua
common.bracketed_paste = function(lines, particularities)
  local result = {}
 -- ~~~local is_ipython = contains(cmd, "ipython")~~~

  lines = remove_empty_lines(lines)

  local indent_open = false
  for i, line in ipairs(lines) do
    if string.match(line, "^%s") ~= nil then
      indent_open = true
    end

    table.insert(result, line)

   -- ~~~if is_windows() and not is_ipython or not is_windows() then~~~
    if particularities["must_close_indents"] then
      if i < #lines and indent_open and string.match(lines[i + 1], "^%s") == nil then
        if not python_close_indent_exceptions(lines[i + 1]) then
          indent_open = false
          table.insert(result, cr)
        end
      end
    end
  end

  -- ~~~if is_windows() then~~~
  if particularities["consider_window_carriage"] then
    table.insert(result, "\r\n")
  else
    table.insert(result, cr)
  end

-- So on and so forth. 
-- If becomes too long one can add a router that will dispatch line formatting to a specific function according to its "particularity"

  return result
end

Then in python.lua one would pass this particularities:

-- python.lua
local bracketed_paste= require("iron.fts.common").bracketed_paste
local python = {}
local is_windows = require("iron.util.os").is_windows

local executable = function(exe)
	return vim.api.nvim_call_function("executable", { exe }) == 1
end

local pyversion = executable("python3") and "python3" or "python"

-- Create a map where each value checks for conditions:
local form_particularities = function(cmd)
  local particularities = {
    ["must_close_indents"] = is_windows() and not contains(cmd, "ipython") or not is_windows(),
    ["consider_windows_carraige"] = is_windows(),
  }
  return particularities
end

local def = function(cmd)
	return {
		command = cmd,
		format = form_particularities(cmd),
	}
end

python.ptipython = def({ "ptipython" })
python.ipython = def({ "ipython", "--no-autoindent" })
python.ptpython = def({ "ptpython" })
python.python = def({ pyversion })

return python

This would keep all language-specific behavior within language-specific files and leave common.lua language agnostic.
I wrote this in a jiffy so it might not be perfect but I think the idea stands. Let me know what you think!

@nickeisenberg
Copy link
Contributor

nickeisenberg commented Jul 9, 2024

@ottersome I like that idea. For the meantime I am going to stick with your original PR. Eventually bracketed_paste_python should be put into fts.python.lua as its not "common" anymore. Your change above with making a more general bracked_paste (which I like better) would require some refactoring else where. Im pretty busy now with work so your first suggesting from the PR will be good for now! Thanks again for the PR, when I get home tonight I will try to get raound to merging it in.

@nickeisenberg nickeisenberg merged commit 87548d7 into Vigemus:master Jul 9, 2024
@nickeisenberg nickeisenberg mentioned this pull request Jul 10, 2024
nickeisenberg added a commit that referenced this pull request Jul 10, 2024
* refactoed the PR so that prior code will not be broken

* fixed a comment in common.format

* added a suggested python configuration to the readme.
@nickeisenberg
Copy link
Contributor

nickeisenberg commented Jul 10, 2024

@ottersome Hey Luis, I wanted to tell you that I slightly refactored you PR in a way that does not break prior users configs.

common.bracketed_paste_python still takes two arguments now, but I edited common.format to send a table to the second argument of the format function. This table can grow as time goes on if need be. But this second argument is optional so it will not break prior users configs as the setup is still

      repl_definition = {
        python = {
          command = { "python3" },
          format = require("iron.fts.common").bracketed_paste_python
        }
      }

@ottersome
Copy link
Contributor Author

Ah,I see! Thank you for the refactor.

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.

2 participants