Discussion sign and diagnostics #78
Conversation
johnybx
force-pushed
the
discussion_sign_and_diagnostics
branch
from
9b4ce93
to
be631f2
Compare
|
I'm good to merge this @johnybx, is that okay with you? Let me know and will do it. |
|
Oh actually, an issue I've noticed -- for some reason when you try and use any other command from Folks should be able to use Diffview outside of the context of this plugin! This is the culprit: M.set_callback_for_file_changed = function(callback)
local group = vim.api.nvim_create_augroup("gitlab.diffview.autocommand.file_changed", {})
vim.api.nvim_create_autocmd("User", {
pattern = { "DiffviewDiffBufWinEnter", "DiffviewViewEnter" },
group = group,
callback = callback,
})
endThese events are triggered when using this plugin (diffview) even when not using |
|
Ah I see, I think I can easily fix it by setting up autocommands at open and then doing cleanup on close. Other option would be to actually check which instance of diffview triggered the autocommand and only conditionally react - that would work even if you opened multiple instances of diffview. I will try to implement it this week 👍 |
johnybx
force-pushed
the
discussion_sign_and_diagnostics
branch
from
6f844be
to
a4b9f3e
Compare
There was a problem hiding this comment.
I updated the auto commands to trigger actions only if it is related to diffview opened by gitlab.nvim and also tried having gitlab review and normal diffview and didn't noticed any errors.
I'll be for sure doing multiple reviews next week so I will be testing more for sure 👍
cmd/git.go
Outdated
| https://git@gitlab.com/namespace/subnamespace/dummy-test-repo.git | ||
| git@git@gitlab.com:namespace/subnamespace/dummy-test-repo.git | ||
| */ | ||
| re := regexp.MustCompile(`(?:https?:\/\/|ssh:\/\/|git@)(?:[^\/:]+)[\/:](.*)\/([^\/]+?)(?:\.git)?$`) |
There was a problem hiding this comment.
I found out that on many repositories I often use very different format of url ( I don't set it manually ) so I updated regexp to match urls in comment. I tested this manually and also here. The main problem was the assumption that namespace is just one word ( ([^\/]+ ) but namespace is everything in URI till the project name.
There was a problem hiding this comment.
Good catch, this was fixed by #87 which caused a merge conflict with this branch, so we can go with that branch's changes here
There was a problem hiding this comment.
I resolved the conflict here but my regexp is more extended - the previous version was for example missing ssh:// and also the .git extension is optional and not necessary ( have few repos like that ). I run tests and nothing was broken - maybe would be worth extend tests for more cases 🤔
There was a problem hiding this comment.
I also added tests.
|
If you make my suggested changes and resolve the merge conflict then I'll merge this! Thank you for dealing with double conflicts here, appreciate this truly awesome contribution! |
johnybx
force-pushed
the
discussion_sign_and_diagnostics
branch
from
a4b9f3e
to
65d4a4a
Compare
|
I resolved conflicts but I don't see your suggested changes 🤔. I see only resolved comments - is it within resolved comments ? |
cmd/git.go
Outdated
| https://git@gitlab.com/namespace/subnamespace/dummy-test-repo.git | ||
| git@git@gitlab.com:namespace/subnamespace/dummy-test-repo.git | ||
| */ | ||
| re := regexp.MustCompile(`(?:https?:\/\/|ssh:\/\/|git@)(?:[^\/:]+)[\/:](.*)\/([^\/]+?)(?:\.git)?$`) |
There was a problem hiding this comment.
Good catch, this was fixed by #87 which caused a merge conflict with this branch, so we can go with that branch's changes here
I'm sorry, the review was never submitted, you should see them now |
👋 Hi,
this PR adds support for discussion sign and diagnostics. Diagnostics and signs are currently split on purpose ( even though the sign can be set also for diagnostic ) to have better control when multiline support will be added ( in this PR ). I will be testing this for few days to see if it works.
TODO:
if we will have period updates maybe it is not great idea to always remove everything and set it again -> it is not hard to update to incremental updates.this does not seem to be neededactions for diagnostics / sign lines ( this may even be new PR )for this PR I added function to jump to discussion tree where all other discussion actions are available.