Skip to content
This repository was archived by the owner on Mar 12, 2024. It is now read-only.

Feature/417 leftout comments#424

Merged
EstebanDalelR merged 11 commits intodevfrom
feature/417-leftout-comments
Jan 24, 2024
Merged

Feature/417 leftout comments#424
EstebanDalelR merged 11 commits intodevfrom
feature/417-leftout-comments

Conversation

@baristaGeek
Copy link
Collaborator

@baristaGeek baristaGeek commented Jan 15, 2024

Description

Adds detecting multi-line leftover comments as a code smell

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Chore: cleanup/renaming, etc
  • RFC
  • Test

Notes

I tested with the commit that this file has that has a leftover comment itself (already removed) and it successfully dects the leftover comment.

However, there is an error in the logic. The code gets the correct position of the occurrence of the leftover comment (number of lines below the @@ header hunk, not the actual line number in the file). However it comments the lineNumber under the wrong header hunk (the first one). Thsi needs to be fixed for this to be upgraded from draft PR.

Acceptance

@vercel
Copy link

vercel bot commented Jan 15, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
watermelon ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 24, 2024 11:07pm

@watermelon-copilot-for-code-review
Copy link

watermelon-copilot-for-code-review bot commented Jan 15, 2024

Watermelon AI Summary

This PR introduces a new feature that enhances the code quality by adding the ability to detect multi-line leftover comments, which are considered a code smell. The series of updates in the PR progressively develop, refine, and streamline this feature, while also addressing an identified error in the logic that incorrectly matches the position of comments within the code.

GitHub PRs

watermelon is an open repo and Watermelon will serve it for free.
🍉🫶
Why not invite more people to your team?

@baristaGeek baristaGeek reopened this Jan 16, 2024
@baristaGeek baristaGeek changed the title TEST - Feature/417 leftout comments Feature/417 leftout comments Jan 16, 2024
@baristaGeek baristaGeek marked this pull request as draft January 16, 2024 05:59
Copy link
Member

@EstebanDalelR EstebanDalelR left a comment

Choose a reason for hiding this comment

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

What can we extract? This is a 600 LOC file!
Code seems ok, but I think we do not want blocking commenting of files but rather choosing one?

@baristaGeek
Copy link
Collaborator Author

What can we extract? This is a 600 LOC file! Code seems ok, but I think we do not want blocking commenting of files but rather choosing one?

I totally agree there's a bunch of things to abstract. I'm worrying about making it work first, then I'll worry about abstracting the code.

@baristaGeek
Copy link
Collaborator Author

I upgraded the PR to "ready for review". Changes include:

  • We now handle the multi-line comments created with "//" on each line case. In addition to the actual multi-line syntax (/* */) case. Image below.
  • We need to index lineNumbers at 4 instead of 0 because the GitHub UI always adds 3 lines below the @@ header (plus they are indexed at 1 instead of 0, plus the octokit comments.position parameter is the number of lines below the @@ heaer instead of the actual line number in the file).
  • I changed the name of the file and the function to detectCodeSmells since we're generalizing the capabilities of this feature.

Considerations

  • Might be missing syntax from some major languages
  • I agree that the file can be abstracted. I propose we agree on whether this works or not, and then the abstraction step could happen in a separate PR.

CC: @EstebanDalelR

Screenshot 2024-01-23 at 4 05 03 PM

const { additions } = getLineDiffs(file.patch ?? "");

// Leftover comment RegEx
// const leftoverCommentRegex = /^\/\*[\s\S]*?\*\//gm;
Copy link
Member

Choose a reason for hiding this comment

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

This seems very independent, can we split it as to call it in this function?
Other than that LGTM!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants