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

Refactor/omnicompletion #620

Merged
merged 6 commits into from
Oct 30, 2023

Conversation

seflue
Copy link
Contributor

@seflue seflue commented Oct 22, 2023

This PR decouples the two modes of omnicompletion, make them easier to understand and reuse. It also adds additional tests and includes some minor improvements (see the particular commits).

- make omni a proper module
- split function omni into two separate methods
- adjust all callers
The added tests cover the wrong behavior (start is one too short for
using # after ::). When the bug get's fixed (or to expose it), the
counter needs to be incremented by 1.
For finer granularity, which helps spotting bugs.
The regex is already hard enough to read. Because \w matches also digits
and underscore, the explicit declarations of both are removed.
'#' is not a special character in vim regex. To simplify the
hard-to-read regex for hyperlinks at least a little bit, we remove these
escapings.
This type hint removes the annoying warning, when passing an url as an
argument to context.fetcher.
@seflue seflue force-pushed the refactor/omnicompletion branch from 36b0a3e to 0c23e22 Compare October 22, 2023 10:33
Copy link
Member

@kristijanhusak kristijanhusak left a comment

Choose a reason for hiding this comment

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

Generally looks ok. I'm just curious why was the regex changed?

@@ -33,8 +32,8 @@ local properties = {
}

local links = {
line_rgx = vim.regex([[\(\(^\|\s\+\)\[\[\)\@<=\(\*\|\#\|file:\)\?\(\(\w\|\/\|\.\|\\\|-\|_\|\d\)\+\)\?]]),
rgx = vim.regex([[\(\*\|\#\|file:\)\?\(\(\w\|\/\|\.\|\\\|-\|_\|\d\)\+\)\?$]]),
line_rgx = vim.regex([[\(\(^\|\s\+\)\[\[\)\@<=\(\*\|#\|file:\)\?\(\(\w\|\/\|\.\|\\\|-\)\+\)\?]]),
Copy link
Member

Choose a reason for hiding this comment

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

Why was this regex changed?

Copy link
Contributor Author

@seflue seflue Oct 26, 2023

Choose a reason for hiding this comment

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

I was hunting a bug (for which I currently only have a workaround), which I suspect to be in the regexp. Because of that I took a deeper dive into the regexp documentation and found out, that it was not the minimal regexp for what it tries to match. Some escapes are unnecessary and the \d and underscore were redundant, because they are already captured by \w. I tried to explain both simplifications in the corresponding commit message. Actually I always try to be as explicit as possible in my commits, so reading my commit messages can be very helpful when reviewing my PRs.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the clarification!

@kristijanhusak kristijanhusak merged commit a14e1e5 into nvim-orgmode:master Oct 30, 2023
4 checks passed
@kristijanhusak
Copy link
Member

Thanks!

@seflue seflue deleted the refactor/omnicompletion branch January 9, 2024 06:38
SlayerOfTheBad pushed a commit to SlayerOfTheBad/orgmode that referenced this pull request Aug 16, 2024
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