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.

Sign up for GitHub

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

Jump to bottom

Fix: Checking whether comment can be created #434

Merged
merged 21 commits into from
Dec 8, 2024
Merged

Fix: Checking whether comment can be created #434

merged 21 commits into from
Dec 8, 2024

Conversation

jakubbortlik
Copy link
Collaborator

Hi @harrisoncramer, in this PR I address #424, by making the following changes:

  1. All checks whether a comment can be created are now made for all three functions: create_comment, create_comment_suggestion and create_multiline_comment in a dedicated function gitlab.actions.comment.can_make_comment().
  2. The checks are made in a more meaningful order (e.g., checking that cursor is in the reviewer is made before the check that current mode is visual mode for multiline comments).
  3. The check for clean working tree before making comments is now correct and it's made on a per-file basis, i.e., unsaved/uncommitted changes in a different file do not block a comment in a good file.
  4. Similarly, with a dirty working tree, choosing a MR is not blocked if that would not lead to switching the branch.
  5. User-specified imply_local = true is overridden in the settings if the working tree is dirty. Before, it was possible that the setting imply_local was true, while the option was not actually applied to the DiffviewOpen command and so commenting was unnecessarily blocked for modified files.

There are some other improvements:

  1. Visual mode is exited when multiline comment creation fails.
  2. The ERROR level is used for all messages when a comment cannot be created.
  3. A draft reply can now be created even if the discussion tree is "detached" from the reviewer.

I'll be grateful if you find the time to review and merge this.

@jakubbortlik jakubbortlik force-pushed the fix-checking-if-comment-can-be-created branch from 0611567 to d3ffd1c Compare December 6, 2024 14:15
@jakubbortlik
fix: return Boolean from has_clean_tree
c35bf81
@jakubbortlik
fix: only look for changes in the current file
1e9bd80
@jakubbortlik
docs: unify error/warning messages
3fea5d2
@jakubbortlik
fix: make dirty tree override imply_local properly
fdb1bfc 
If `imply_local` is set but unused due to local changes present when review is started,
`imply_local` is overridden so that local changes do not unnecessarily block comments.

Now, uncommitted changes on a file will only block comments when they are made after the review is
started with `imply_local` applied.
@jakubbortlik
perf: only check tree cleanness with imply_local
1eb187b
@jakubbortlik
fix: only block choose_merge_request on dirty tree when switching branch
77f79db
@jakubbortlik
docs: use more explicit error message
95ba38a
@jakubbortlik
refactor: move logic to separate function
a7c66ee
@jakubbortlik
refactor: move sha_exists check to common function
474d58f
@jakubbortlik
fix: check for modification in common function
6fa3e92
@jakubbortlik
docs: use more complete warning
b381bee
@jakubbortlik
refactor: move mode checking to common function
3bbe588
@jakubbortlik
fix: check correct window before checking file renamed
460fc6b
@jakubbortlik
perf: remove unnecessary nil checks
180e53c 
The function create_comment_layout should now always return a layout (it used to return nil if the
comment could not be created at all).
@jakubbortlik
fix: exit visual mode when comment cannot be created
c99a16d
@jakubbortlik
perf: remove unnecessary check
e24bb66 
The check that `diffview_lib.get_current_view() == nil` is superfluous because we later also check
explicitly that we are in the reviewer tab.
@jakubbortlik
docs: use more explicit comments
c83ca0a
@jakubbortlik
fix: check for nil
f0e5dc7
@jakubbortlik
fix: don't require position_data for draft replies
d286641 
The position data are not necessary for creating a draft reply and requiring it prevented draft
replies in case the discussion tree was open outside of the reviewer pane.
@jakubbortlik
fix: use correct message level for comment creation errors
4a6f219
@jakubbortlik
style: remove unused variable
3479bde
@jakubbortlik jakubbortlik force-pushed the fix-checking-if-comment-can-be-created branch from d3ffd1c to 3479bde Compare December 8, 2024 13:57
@harrisoncramer harrisoncramer merged commit 508a394 into harrisoncramer:develop Dec 8, 2024
6 checks passed
@harrisoncramer
Copy link
Owner

This looks great, thank you for the contribution. The refactor and consolidation of the comment checks was needed.

harrisoncramer added a commit that referenced this pull request Dec 11, 2024
@harrisoncramer @jakubbortlik
Release (#440)
495e64c 
* Feat: Enable sorting discussions by original comment (#422)
* Feat: Improve popup UX (#426)
* Feat: Automatically update MR summary details (#427)
* Feat: Show update progress in winbar (#432)
* Feat: Abbreviate winbar (#439)
* Fix: Note Creation Bug (#441)
* Fix: Checking whether comment can be created (#434)
* Fix: Syntax in discussion tree (#433)
* fix: improve indication of resolved threads and drafts (#442)
* Docs: Various minor improvements (#445)

---------

Co-authored-by: Jakub F. Bortlík <[email protected]>
@jakubbortlik jakubbortlik deleted the fix-checking-if-comment-can-be-created branch December 12, 2024 06:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jakubbortlik @harrisoncramer