Shatur / neovim-cmake

CMake integration for Neovim
GNU General Public License v3.0
87 stars 19 forks source link

Enable user to define a directory to run the targets from #4

Closed jxi24 closed 3 years ago

jxi24 commented 3 years ago

Fixes #3.

The neovim.json file now has the option for the user to define the directory to run the target from. If this is omitted, then the directory defaults to the directory containing the current target.

Furthermore, the README.md was updated to include this additional information. I also added a dependency on the plenary plugin to make the parsing of the path easier and to allow for logging of messages to help trace through the code. If this is not desired, I can rework the logging and path parsing to not use plenary.

Shatur commented 3 years ago

I would use use camelCase for JSON options as CMake do.

if this is not desired, I can rework the logging and path parsing to not use plenary.

I thank that introduce logging system for a just one message is too much :D I would add a logging system for all stuff later. If you just need to quickly debug something for now, Neovim has vim.notify(). But I don't mind to left Plenary dependency for path parsing since it almost standard plugin in every workflow. Also should we specify the folder relatively to the target location? Maybe the run directory should be relative to the build folder?

jxi24 commented 3 years ago

I was planning on adding more logging, but did not get around to it last night. I can remove it, if you had your own plans for a logger.

This code: https://github.com/jxi24/neovim-cmake/blob/3a6e85e49d8b313755bb45cb5ab09f7e03c623bc/lua/cmake/utils.lua#L98-L99 creates a relative path from the runDir that the user has defined. Right now, the runDir needs to be a full path, I can modify it so that it takes in a relative path from the build folder if you prefer, or check to see if it has a leading / to determine if it is a full path or a relative path.

Shatur commented 3 years ago

I was planning on adding more logging, but did not get around to it last night. I can remove it, if you had your own plans for a logger.

Yes, it would be nice to keep the scope of the PR simple.

check to see if it has a leading / to determine if it is a full path or a relative path.

I think it would be great. Plenary have is_absolute check, we can use it. If it return false, then let's make it relative to the build dir:

https://github.com/jxi24/neovim-cmake/blob/3a6e85e49d8b313755bb45cb5ab09f7e03c623bc/lua/cmake/utils.lua#L83

jxi24 commented 3 years ago

I updated the code to now handle the relative or absolute paths. I tested out a few paths, and it seems to work as expected. I also removed all the logging information

jxi24 commented 3 years ago

I agree that we are good to go.