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: option to disable #85

Merged
merged 6 commits into from
May 18, 2024

Conversation

FireIsGood
Copy link
Contributor

Refactors config into a new file and then adds the option of notifyNotFound (default of true) to allow disabling the notification.

  • Moved init.lua configuration → configuration.lua
  • Added notifyNotFound to configuration (default: true)

Example:

Peek.2024-05-18.00-53.mp4

Checklist

  • Used only camelCase variable names.
  • If functionality is added or modified, also made respective changes to the
    README.md (the .txt file is auto-generated and does not need to be modified).
  • Used conventional commits keywords.

@FireIsGood
Copy link
Contributor Author

An additional consideration might be to change the individual declarations of config.lookForwardSmall and config.lookForwardBig to be declared at the top of the file as the line-wise file seems to only use the Big size while the character-wise file seems to only use the Small size.

Copy link
Owner

@chrisgrieser chrisgrieser left a comment

Choose a reason for hiding this comment

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

Just an issue with the setup call, otherwise nice work

if M.config.useDefaultKeymaps then
require("various-textobjs.default-keymaps").setup(M.config.disabledKeymaps)
end
return M.config
Copy link
Owner

Choose a reason for hiding this comment

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

Don't think this needs a return value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the implementation I went with, the setup() function both modifies the module's config attribute and also returns a copy of it. If I were to use the setup() only to set the user config, assigning the config could be done with the config attribute instead in the init.lua call.

Would this be preferred?


---optional setup function
---@param userConfig? config
function M.setup(userConfig)
config = vim.tbl_deep_extend("force", defaultConfig, userConfig or {})
local config = require("various-textobjs.config").setup(userConfig)

if config.useDefaultKeymaps then
Copy link
Owner

Choose a reason for hiding this comment

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

The default keymaps are now created twice

@chrisgrieser
Copy link
Owner

An additional consideration might be to change the individual declarations of config.lookForwardSmall and config.lookForwardBig to be declared at the top of the file as the line-wise file seems to only use the Big size while the character-wise file seems to only use the Small size.

I think there were 1-2 exceptions to this, iirc the url text object.

Previously, it returned a copy of the module. This is likely a bad
pattern so it has been replaced by just having the user call the config
attribut instead.
Copy link
Owner

@chrisgrieser chrisgrieser left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@chrisgrieser chrisgrieser merged commit 2e9982c into chrisgrieser:main May 18, 2024
1 check passed
@chrisgrieser
Copy link
Owner

btw, the returning of the config is not really a bad pattern, the return value is just unnecessary because it works have been never used, that is all

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