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

Release v2.0.0 #136

Merged
merged 28 commits into from
Aug 15, 2022
Merged

Release v2.0.0 #136

merged 28 commits into from
Aug 15, 2022

Conversation

kylechui
Copy link
Owner

@kylechui kylechui commented Aug 11, 2022

Breaking Changes

TODO: In the actual release notes, just link to #77

Some user configuration variables have been renamed to better reflect their actual purpose:

  • highlight_motionhighlight
  • NvimSurroundHighlightTextObjectNvimSurroundHighlight
  • The textobject key for require("nvim-surround.config").get_selection() has been replaced by the motion key. This allows for better compatibility with targets.vim and similar plugins that define their own text-objects. All existing textobject keys just need to be pre-pended with a to represent the correct motion, e.g.
-- Old configuration
require("nvim-surround.config").get_selection({
    textobject = ")",
})
-- New configuration
require("nvim-surround.config").get_selection({
    motion = "a)",
})

New Features

  • Adds first-class Tree-sitter support for finding parent selections; see :h nvim-surround.config.get_selection()
    • This allows for dsf to better handle edge cases like smiley_face(":)")
  • Filetype-specific surrounds (that utilize Tree-sitter) can be set by default
  • nvim-surround indentation can be disabled by setting the new key indent_lines = false; see :h nvim-surround.config.indent_lines

Bug Fixes

Future Goals

kylechui and others added 10 commits August 9, 2022 16:27
* feat!: Easier user configuration.

* Breaking change: The translation layer first falls back on any defined
  defaults, before using `invalid_key_behavior`.

* docs: Improve code docs for `config`.

* refactor!: Rename `textobject` to `motion`.

Breaking Change: The `textobject` key for the `config.get_selection()`
function has been renamed to `motion`, to allow for more generalized
surrounds. As a result, all existing `textobject` values should have `a`
pre-pended to them.

* refactor!: Rename the highlight group.

Breaking Change: Renames the highlight group from
`NvimSurroundHighlightTextObject` to `NvimSurroundHighlight`.
* Remaining issues: Visual block mode.
* Remaining issues: Does not surround the literal visual block.
* feat: Add native Tree-sitter support.

* Documents the new feature.
* Changes default behavior for `f` to use Tree-sitter nodes by default,
  falling back on Lua patterns.
* Refactors the "nearest selection" heuristic into its own function.

* feat: Improve HTML tag/function call patterns.

* feat: Sample filetype config for Lua files.

* feat: Improve pattern-matching Tree-sitter nodes.

* feat!: Add `languages` file with new API.

* fix: Naming bug.

* refactor: Rename `languages` to `filetype`.

* docs: Comment code.
vim.api.nvim_create_autocmd("FileType", {
pattern = vim.tbl_keys(M.filetype_configurations),
callback = function()
M.filetype_configurations[vim.bo.filetype]()
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like there should be a nil-check here in case there is no function for the given filetype :)

Copy link
Owner Author

@kylechui kylechui Aug 13, 2022

Choose a reason for hiding this comment

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

@smjonas I actually forgot I accounted for this, but there will always exist a function for the given filetype, since the autocommand uses the M.filetype_configurations table to even choose which files to run on. So this shouldn't be an issue as far as I can tell :) Thanks for checking though!

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, you're right, I missed that :D

Copy link

@latipun7 latipun7 left a comment

Choose a reason for hiding this comment

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

---@field add function
---@field find function
---@field delete function
---@field change { target: function, replacement: function? }

Those lines causing LSP problems as shown below.
image

Please add string type to find, delete, and target fields and add string[] to the add field since documentation said that those fileds can accept string type while add field can take the list of string.

@kylechui
Copy link
Owner Author

kylechui commented Aug 14, 2022

Hmm I didn't realize that LSP would actually tell you about that; the reason why I initially omitted them is because when nvim-surround stores everything in a buffer-local variable, it actually "translates" all user configuration to a standardized, internal format. For example add = { "(", ")" } gets converted to add = function() return { { "(" }, { ")" } } end. For the time being this will work just fine, but I wonder if I should create two sets of type annotations, one for user-facing configuration and one for the internal workings. Thanks for the comment @latipun7

@smjonas
Copy link
Contributor

smjonas commented Aug 15, 2022

One suggestion for this release:
For custom surrounds, it is often the case that delete and change.target need to be set to the same pattern. I would even say that this is the case for the majority of surrounds. Examples: simple character surrounds such as surrounding with ** or a function call such as pretty_print in Lua.

Would you consider falling back to the delete pattern if change.target is nil? This would reduce code duplication for the user and simplifies the API. It would still be possible to disable change entirely if set to an empty function, right?

Example:
Old:

    p = {
      add = { "vim.pretty_print(", ")" },
      find = "vim%.pretty_print%b()",
      delete = "^(vim%.pretty_print%()().-(%))()$",
      change = {
        target = "^(vim%.pretty_print%()().-(%))()$",
      },
    },

New:

    p = {
      add = { "vim.pretty_print(", ")" },
      find = "vim%.pretty_print%b()",
      delete = "^(vim%.pretty_print%()().-(%))()$",
    },

csp will still work as before.

Let me know what you think!

@kylechui
Copy link
Owner Author

@smjonas Very good point. I suppose change is only different from a literal add/delete pair if it's more complex (e.g. function call, HTML tag), and isn't actually necessary most of the time. I'll add this in at some point; still working on user interface for default filetype-specific surrounds. Design is hard 😅

* By default, `change.target` will take on the `delete` key's values,
  analogous to how `change.replacement` will take on the next surround's
  `add` key's value.
@kylechui
Copy link
Owner Author

@smjonas Added in the latest commit, thanks for the great suggestion!

@smjonas
Copy link
Contributor

smjonas commented Aug 15, 2022

Awesome, thank you for implementing that so fast!

@kylechui
Copy link
Owner Author

If you actually look at the commit it was honestly a really quick fix, just added like 3-4 lines :)

@kylechui kylechui merged commit cefbfa5 into main Aug 15, 2022
@kylechui kylechui deleted the v2.0.0 branch August 15, 2022 22:30
@kylechui
Copy link
Owner Author

Oh wait @smjonas an interesting (new) issue cropped up as a result of defaulting to delete; in the case that a buffer-local setup does not define change.target, should it default to that surround's delete key, or the global surround's change.target key? In particular, if the f surround for Lua files is set by

require("nvim-surround").buffer_setup({
    surrounds = {
        ["f"] = {
            find = function()
                return config.get_selection({ node = "function_call" })
            end,
            delete = "^([^=%s]+%()().-(%))()$",
        },
    },
})

then csf will include the closing parenthesis, since it is falling back on the delete key. I'm not sure if changing it to look globally first will cause any other unforeseen issues though :/

@smjonas
Copy link
Contributor

smjonas commented Aug 15, 2022

I would suggest to only fallback to the local surround's delete key. This way, users can immediately tell what will happen without having to look at the global configuration first. My suggestion was mainly intended to reduce duplicate code within a single surround.

@kylechui
Copy link
Owner Author

kylechui commented Aug 15, 2022

Hmm... this behavior might actually be unrelated; will do some more investigating.

Edit: It was unrelated. Oops!

@kylechui
Copy link
Owner Author

@smjonas I've decided on falling back on globals first, since I don't think it makes sense that tweaking delete for function calls for the current filetype should affect tweaking change.target for the same filetype. The omission of the change key is just syntactic sugar, but they should remain separate I feel.

@latipun7
Copy link

Great 🎉
Waiting for this PR appeared in a release tag 👍 (since I install this plugin pinned to latest tag 😃)

@kylechui
Copy link
Owner Author

@latipun7 There are (unfortunately) still quite a few bugs that need to be ironed out, as well as user config interface decisions that need to be made. This most likely will not appear in a release for a little bit, unless something magical happens and I'm satisfied with the quality of the code.

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