Skip to content
This repository has been archived by the owner on Sep 19, 2024. It is now read-only.

Blame #778

Open
wants to merge 7 commits into
base: development
Choose a base branch
from
Open

Blame #778

wants to merge 7 commits into from

Conversation

Keyrxng
Copy link
Member

@Keyrxng Keyrxng commented Sep 17, 2023

Resolves #758

Quality Assurance:

The diff comparison works by finding the merge_commit_sha and compares it to currentCommit.sha

Looking for opinions going forward from here in regards to identifying who broke the feature of the issue.

We can go direct and say 'who changed the code from pr a to pr b' and that's who is at fault but what if the feature of the issue gets broken by a peripheral change, how do we identify that? (The more I think about it this will likely be in the AI infused version am I right?)

Or do we approach it from a 'this was the last working version' so last man on top carries the burden?

If any of the original assignee's code was changed from the time it was approved to the current production version (using diff this should be easy to see) then the blame should no longer be on the original assignee.

Your spec sounds a lot like 'pr a to pr b' for me, so my next steps (at least for the first iteration) will be simply looking for any commits that overwrite the implementation of the issue.

I'm thinking if there are multiple contributors then it'll just need to boil down to whoever changed the most code?

initial stages needing input
@netlify
Copy link

netlify bot commented Sep 17, 2023

Deploy Preview for ubiquibot-staging ready!

Name Link
🔨 Latest commit e7d5ff1
🔍 Latest deploy log https://app.netlify.com/sites/ubiquibot-staging/deploys/652b4cf2eb6fab0008b7eb38
😎 Deploy Preview https://deploy-preview-778--ubiquibot-staging.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@Keyrxng
Copy link
Member Author

Keyrxng commented Sep 17, 2023

@pavlovcik Bit busy the past few days but finally got around to it, let me know what your thoughts are when you can

@0x4007
Copy link
Member

0x4007 commented Sep 18, 2023

Let's do two dot compare instead of three dot. I think it shows every file change. To be honest I'm not 100% clear on the difference but one seems to be more granular.

I think for peripheral change we can figure an improvement to the feature with a new bounty.

Not all lines of code changed are equal so that's why I don't want to "blame more" who changed the most lines of code. However in the blame list we could sort by most lines of code changed to least but I don't think we need to emphasize this. As long as they changed any lines from the merged pull request, they are a suspect.

This is cool to see currently.

Copy link
Member

@0x4007 0x4007 left a comment

Choose a reason for hiding this comment

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

Code and qa looks pretty solid so far

src/handlers/processors.ts Show resolved Hide resolved
@Keyrxng
Copy link
Member Author

Keyrxng commented Sep 18, 2023

Let's do two dot compare instead of three dot. I think it shows every file change. To be honest I'm not 100% clear on the difference but one seems to be more granular.

The three-dot comparison shows the difference between the latest common commit of both branches (merge base) and the most recent version of the topic branch.

A two-dot diff compares two Git committish references, such as SHAs or OIDs (Object IDs), directly with each other. On GitHub, the Git committish references in a two-dot diff comparison must be pushed to the same repository or its forks.

Three dot gets the benefit of the GitHub UI prettifying it for us. In order to get a two dot comparison we'll need to handle formatting the url ourselves.

Currently: ref...ref and it works

Incoming: Use the compareChanges url with dual dots and insert the refs ourselves, but how should it be formatted?

https://github.com/Keyrxng/UbiquityBotTesting/issues/47#issuecomment-1722344751

assorted blame
@Keyrxng Keyrxng marked this pull request as ready for review October 15, 2023 00:07
@Keyrxng
Copy link
Member Author

Keyrxng commented Oct 15, 2023

Now showing the reviewers, which PRs they reviewed as well the count being removed from hunters. Included a comparison for all merge commits and left in the example 2 and 3 dot comparisons lmk which of the two is the preference.

Where do I take it from here?

@0x4007
Copy link
Member

0x4007 commented Oct 15, 2023

I fear that merged pulls and commits lists could get massive but I can't think of a better solution now.

Seems pretty solid so far.

As a heads up we are temporarily pausing merged into development until I can get my massive refactor in (I have a draft PR now)

Furthermore we plan to only accept with unit tests so if you want to try and get a head start feel free or you can standby for another week to add when jest is merged in the project

Copy link
Member

@0x4007 0x4007 left a comment

Choose a reason for hiding this comment

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

Using null is more expressive than "" to signal that it is intentionally an empty value.

Besides it's less verbose to check conditional statements as well, given that null is falsely and "" is truthy

You should apply this everywhere applicable

src/handlers/comment/handlers/index.ts Outdated Show resolved Hide resolved
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.

Penalty & Blame For Broken Features
3 participants