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

feat!: better defaults for copy_directories + only add existing ones #31

Merged
merged 1 commit into from
Mar 8, 2023

Conversation

mrcjkb
Copy link
Member

@mrcjkb mrcjkb commented Mar 1, 2023

BREAKING CHANGE: This could result in new directories being added to packages that did not set the copy_directories input.

@mrcjkb mrcjkb linked an issue Mar 1, 2023 that may be closed by this pull request
@mrcjkb mrcjkb requested a review from teto March 1, 2023 23:20
@mrcjkb mrcjkb marked this pull request as draft March 1, 2023 23:24
@mrcjkb
Copy link
Member Author

mrcjkb commented Mar 1, 2023

I've noticed that :help runtimepath does not include some common plugin directories like autoload, queries, ...

Since adding new defaults is technically a breaking change, we should probably as for feedback on which directories to add in some Neovim groups.

Update: Nevermind. autoload is there. It's just queries, which is a directory used by nvim-treesitter.
This PR should be ready now.

@mrcjkb mrcjkb force-pushed the 15-more-adequate-defaults-for-copy_directories branch from 4c3dbfb to e0077d9 Compare March 2, 2023 14:35
@mrcjkb mrcjkb marked this pull request as ready for review March 2, 2023 14:49
@teto
Copy link
Member

teto commented Mar 2, 2023

my experience with CI is that the more code you move to the script and the less you expose in the CI-only part is better. It makes troubleshooting easier. In the long run, I think it's better to implement the default directly in luarocks-tag-release.lua.

As you mentioned you wanted the action to remain generic, maybe instead we could have an input: "neovimplugin" (autodetected according to label even) that would change some settings:

  • add "neovim" label
  • add these folders to copy_directories

@mrcjkb
Copy link
Member Author

mrcjkb commented Mar 3, 2023

my experience with CI is that the more code you move to the script and the less you expose in the CI-only part is better. It makes troubleshooting easier. In the long run, I think it's better to implement the default directly in luarocks-tag-release.lua.

As you mentioned you wanted the action to remain generic, maybe instead we could have an input: "neovimplugin" (autodetected according to label even) that would change some settings:

* add "neovim" label

* add these folders to copy_directories

I see your point about troubleshooting.
But I'm not sure how I feel about a separate input or logic that causes directories to be added on top of the copy_directories input based on some condition. That sounds to me like it could increase complexity (both internally and from a UX viewpoint). I might be wrong though...

Another option would be to add the default plugin directories if the copy_directories input has the value {{ neovim.plugin.dirs }} (which we could set by default).

I'll put some more thought into it in the next few days.

@mrcjkb mrcjkb force-pushed the 15-more-adequate-defaults-for-copy_directories branch 2 times, most recently from 7b8298d to d6cb786 Compare March 4, 2023 13:27
@mrcjkb
Copy link
Member Author

mrcjkb commented Mar 4, 2023

I have updated this with the {{ neovim.plugin.dirs }} approach.

@mrcjkb mrcjkb force-pushed the 15-more-adequate-defaults-for-copy_directories branch 2 times, most recently from b90e6f8 to f6fb7c5 Compare March 8, 2023 13:31
BREAKING CHANGE: This could result in new directories being added
to packages that did not set the copy_directories input.
@mrcjkb mrcjkb force-pushed the 15-more-adequate-defaults-for-copy_directories branch from f6fb7c5 to 630df48 Compare March 8, 2023 13:32
@mrcjkb mrcjkb merged commit 61eed1c into master Mar 8, 2023
@mrcjkb mrcjkb deleted the 15-more-adequate-defaults-for-copy_directories branch March 8, 2023 13:34
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.

more adequate defaults for "copy_directories"
2 participants