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

Adding an option to allow automatic shebang even when current dir is not in $PATH #3

Open
shreyas-a-s opened this issue Apr 4, 2024 · 19 comments

Comments

@shreyas-a-s
Copy link

shreyas-a-s commented Apr 4, 2024

Hi, Nogueras. First of all, great plugin you have created. Works like a charm.

I would like the addition of an option with which even if current directory is not in $PATH, automatic shebang is applied.

Maybe something like modifying already existing variable automatic to have an extra value ignore_path would be possible I think.

I see that there is already a PR i.e. #1 by @mgabs but it only adds a new variable and nothing more as far as I can see.

I would love to contribute in adding this feature if you would like that and if the scope of this plugin allows such a feature. If not, then no problem at all.

Thanks for reading through my essay. Hoping to hear from you.

@Susensio
Copy link
Owner

Susensio commented Apr 4, 2024

Hi @shreyas-a-s , thanks you for your nice words.

If I understand the issue correctly, you would like to automatically add shebang to ALL new files?

@shreyas-a-s
Copy link
Author

shreyas-a-s commented Apr 4, 2024

@Susensio Yes, Exactly. Irrespective of whether the file is in $PATH or not.

EDIT: That's because I usually create bash, python, etc. scripts in non-PATH directories. For example in /home/shreyas/test. I would like the shebang to be automatically applied when creating new files in that directory.

I know it is possible with autocommands. But an inbuilt option would be much simpler.

@Susensio
Copy link
Owner

Susensio commented Apr 5, 2024

I find it interesting that this feature is being proposed twice already. I would like understand the use case better to get proper implementation.

What if you are working in a large python project? By large I mean multiple modules. Everytime you edit a .py file, a shebang would be inserted and the file would be made executable. I think that behavior would be unintended.

Maybe an interesting solution would be to add custom paths as an option, maybe using glob patterns.
That way you could define additional paths beside PATH.
What do you think? Would it work for you?

BTW: you could always use the :Bang command, maybe keymaping it to a shorter combination.

@shreyas-a-s
Copy link
Author

shreyas-a-s commented Apr 6, 2024

The 'adding shebang' part only works for new (python) files, right? If so I don't think it would be that problematic.

But I really forgot about the 'making it executable' part. Thanks for letting me know. That might bring some issues.

Hence I think the custom paths option that takes in a table of globs would be the desired solution.

Because for my use case, currently I only need this new feature to work on .sh files in a specific directory (for example ~/test) hence I think the glob that I might use would be ~/test/*.sh.

I don't know much about the programming implications of adding such a feature. I'm still learning. But I would love to help in any manner in making this feature possible.

EDIT: About the :Bang command, I was using keymap for invoking it and now using an autocommand for the same. It's just that since I am trying to reduce as much as friction from coding as possible, I thought automating the process of adding a shebang would be pretty cool and an inbuilt option would be even cooler.

@Susensio
Copy link
Owner

Sorry for the late replay. If you like to, you can implement it and send a PR. I was thinking in an extra_paths config that accepts simple globs.

@shreyas-a-s
Copy link
Author

It's absolutely fine for the late reply. We all have other things to do right! The fact that you have replied is the important part.

extra_paths seems good to me too. Since I am still a beginner when it comes to programming, I don't know how good my coding would be nor would it work. But with a bit of help from your side, I think I can do it. I will implement it and send the PR. Then we can discuss and modify it accordingly. Is that ok with you?

@Susensio
Copy link
Owner

That's perfect. If you need any help understanding my code, just ask.

@shreyas-a-s
Copy link
Author

For sure.

@shreyas-a-s
Copy link
Author

Hi, @Susensio.

I think I have a basic configuration of the feature. All the variable, function names as well as the totality of the code can be considered as an initial draft and any modification can be made. You can checkout this commit to see the changes.

Also, I had some doubts regarding the feature:

  1. Obviously, my doubt of whether this code that I wrote is any good? Please check it out and comment on it.
  2. When it comes to the extra_paths table, are we hoping to use lua globs or the normal linux globs? What I mean is, if one had a directory /home/username/test and he wanted to apply shebang to all .sh files in there, should he be able use /home/username/test/*.sh or /home/username/testing/.*%.sh (the lua way of specifying globs)? The lua way is the simplest to implement (it is the way I have done it), but I think the normal linux globs would be better from a user-experience stand point.

Thanks for letting me contribute to the project.

@Susensio
Copy link
Owner

Susensio commented May 9, 2024

Hey Shreyas, thanks for your work.
Your implementation seems good.

Regarding your concern about globs, I think globs are more user friendly than lua patterns, and I would rather use them. But the implementation may be simpler! Autocmd patterns [:h autocmd-pattern] are close enough to shell globs. Then, check_against_globs would not be needed. Only the autocmd.

I haven't tested it but I think it should work.
What do you think?

@Susensio
Copy link
Owner

Susensio commented May 9, 2024

Now that I think about it, are globs even needed here?
I mean, if extra paths is a list of directories, that would be more consistent with system PATH.
e.g.

extra_paths = {
  "~/bin",
  "~/coolproject/scripts/"
}

That should get translated into

vim.api.nvim_create_autocmd(
  "BufNewFile",
  { 
    pattern = {
      "~/bin/*",
      "~/coolproject/scripts/*"
    }
    ...
  }
)

@shreyas-a-s
Copy link
Author

Hey Shreyas, thanks for your work. Your implementation seems good.

Regarding your concern about globs, I think globs are more user friendly than lua patterns, and I would rather use them. But the implementation may be simpler! Autocmd patterns [:h autocmd-pattern] are close enough to shell globs. Then, check_against_globs would not be needed. Only the autocmd.

I haven't tested it but I think it should work. What do you think?

Thanks for the good words, man. It truly motivates me.

Regarding globs-or-patterns, I agree that globs are more user-friendly and generally better. And about the autocmd pattern matching, thank you so much for letting me remember. It had totally gone through the top of my head that there is a pattern = '*' in even in the code I have written and that it accepts a table like pattern = {}. So definitely we can leverage that and it will allow us to remove the check_against_globs function and thereby making the code a lot simpler. I'll implement that change.

@shreyas-a-s
Copy link
Author

Now that I think about it, are globs even needed here? I mean, if extra paths is a list of directories, that would be more consistent with system PATH. e.g.

extra_paths = {
  "~/bin",
  "~/coolproject/scripts/"
}

That should get translated into

vim.api.nvim_create_autocmd(
  "BufNewFile",
  { 
    pattern = {
      "~/bin/*",
      "~/coolproject/scripts/*"
    }
    ...
  }
)

That's totally true. If extra_paths is a table of directories then we just need to code the addition of * to the end of them and we can directly use the table something like pattern = M.config.extra_paths.

The reason I brought up the use of globs was so that the application of shebang can be made a bit more granular. What I mean by that is, if we allow only directories to be placed inside extra_paths table, then a user can choose a folder and shebang will be applied to all new files in it. But if we allow globs, the user can explicitly choose whether to apply shebang to all files i.e extra_paths = { "~/newfolder/*" } or just for one particular type of file extra_paths = { "~/newfolder/*.extension" }.

But now that I think more, this scenario is a bit too niche to even think about it. So I am also getting to a point where I think we should just roll with 'allowing only directory paths' in the extra_paths table. Right? I would greatly appreciate your point of view on this.

@Susensio
Copy link
Owner

But now that I think more, this scenario is a bit too niche to even think about it. So I am also getting to a point where I think we should just roll with 'allowing only directory paths' in the extra_paths table. Right? I would greatly appreciate your point of view on this.

What is your personal use case? Would you use globs per filetype?


Two scenarios I see here:

only directories

  1. Prevent errors: ensure no glob * is in extra_paths, error and warn otherwise
  2. Normalization: for each extra paths (on saving config) remove last forward slash / if present.
  3. Make pattern: on creating autocmd, add /* to each extra_path to make the pattern work

This seems doable and easy

allow globs

  1. Detect when should each extra_path be modified or left as is. This is not trivial. And can lead to unexpected behaviour.
    -- Maybe search for * in the string? But * could be in a parent path such as */bin and that should be converted into */bin/*
    -- Only search * in the last component of path? /foo/bar* could mean a script /foo/bar_64 or a folder /foo/bar_64/*

  2. If the above can be accomplished, normalize and make pattern when needed


If you don't think you would use globs, I would rather keep it simple and only allow directory paths. The other option is... too magic I think

@shreyas-a-s
Copy link
Author

shreyas-a-s commented May 11, 2024

To be honest, now I really don't know where the thought of globs came from. I don't think I would need globs. Because my use case is that I have a directory that is ~/tests and I want the new shell/python scripts I create inside that directory to have shebang applied automatically. That's all I wanted in the first place.

So at the moment there is not really a good reason to opt for globs, especially since it requires more work.

Hence let's go with allowing only directories in the table.

Now when it comes to implementation part

  1. Prevent errors: ensure no glob * is in extra_paths, error and warn otherwise
  2. Normalization: for each extra paths (on saving config) remove last forward slash / if present.
  3. Make pattern: on creating autocmd, add /* to each extra_path to make the pattern work
  1. How are you thinking of implementing that? Detecting * in the table items and doing a print() or vim.notify() is what comes to my mind. Is there any other way of doing that?
  2. This means creating a function (maybe even named normalise_paths) and removing all the trailing slashes, right?
  3. For adding /* to end of each item in extra_paths, create another function or is there any better alternatives to doing so?

Also, I think it would also be necessary to convert '~' in items to actual HOME path using something like the code below so that users don't need to write the full path to home directory.

  local home = os.getenv("HOME")
  modified_path = path:gsub("^~", home)

@Susensio
Copy link
Owner

Susensio commented Jun 3, 2024

To be honest, now I really don't know where the thought of globs came from. I don't think I would need globs. Because my use case is that I have a directory that is ~/tests and I want the new shell/python scripts I create inside that directory to have shebang applied automatically. That's all I wanted in the first place.

So at the moment there is not really a good reason to opt for globs, especially since it requires more work.

Hence let's go with allowing only directories in the table.

Now when it comes to implementation part

  1. Prevent errors: ensure no glob * is in extra_paths, error and warn otherwise
  2. Normalization: for each extra paths (on saving config) remove last forward slash / if present.
  3. Make pattern: on creating autocmd, add /* to each extra_path to make the pattern work
  1. How are you thinking of implementing that? Detecting * in the table items and doing a print() or vim.notify() is what comes to my mind. Is there any other way of doing that?
  2. This means creating a function (maybe even named normalise_paths) and removing all the trailing slashes, right?
  3. For adding /* to end of each item in extra_paths, create another function or is there any better alternatives to doing so?

Also, I think it would also be necessary to convert '~' in items to actual HOME path using something like the code below so that users don't need to write the full path to home directory.

  local home = os.getenv("HOME")
  modified_path = path:gsub("^~", home)

Hi @shreyas-a-s, and sorry again for the delay.
About your points:

  1. vim notify seems a good idea. I would use a low level logging level and let the user know that glob usage is discouraged. I think we don't need to fail, just warn.
  2. sounds good
  3. maybe just string concatenation som_var .. "/"

I think that ~ is handled already in autocmd pattern, but I could be wrong.

@shreyas-a-s
Copy link
Author

shreyas-a-s commented Jun 4, 2024

Again, it's absolutely no problem, mate :)

  1. I might need a bit help in that.
  2. Cool
  3. Yeah, that'll work

From my testing ~ is not handled by autocmd, but I'll look into it a bit more.

At this point, I feel we've touched all the important points. I think It'd be best if I create a PR and we discuss and modify that. You ok with that?

@Susensio
Copy link
Owner

Susensio commented Jun 6, 2024

  1. I might need a bit help in that.

something like vim.notify("Our message...", vim.log.levels.DEBUG) should do it

From my testing ~ is not handled by autocmd, but I'll look into it a bit more.

I've read in :h autocmd-pattern that ~ should be expanded, but right now i'm having a problem in my config with this same issue and I think you may be right, idk... I'll take a look as well

And ~ can be used for the home directory (if $HOME is defined): >
	:autocmd BufWritePost ~/.config/nvim/init.vim   so <afile>
	:autocmd BufRead ~archive/*      set readonly
The environment variable is expanded when the autocommand is defined, not when
the autocommand is executed.  This is different from the command!

At this point, I feel we've touched all the important points. I think It'd be best if I create a PR and we discuss and modify that. You ok with that?

perfect, give it a go!

@Susensio
Copy link
Owner

Susensio commented Jun 7, 2024

:h nvim_create_autocmd

You were right, thanks for pointing that out, I've just solved a bug in my config =)

Note: pattern is NOT automatically expanded (unlike with :autocmd), thus names like "$HOME" and "~" must be expanded explicitly:
pattern = vim.fn.expand("~") .. "/some/path/*.py"

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

No branches or pull requests

2 participants