-
Notifications
You must be signed in to change notification settings - Fork 1
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
feat(quickie): only gitfile should have quickie #1042
feat(quickie): only gitfile should have quickie #1042
Conversation
Current dependencies on/for this PR: This stack of pull requests is managed by Graphite. |
ce03cb0
to
b9701ed
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm actually fine with this as-is, as long as we just remove the implementation in repo service with super.<new function in github service>
so that it's clear where stuff is implemented
@@ -604,16 +603,175 @@ export default class GitHubService { | |||
return newCommitSha | |||
} | |||
|
|||
async updateRepoState( | |||
async deleteDirectory( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this method is functionally identical to the one in repoService
- we could probably delete either this or replace the reposervice implementation with a super.deleteDirectory
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch!
link to diff checker as a sanity check: https://www.diffchecker.com/4HWiYNIH/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not entirely sure wym here - the diff checker shows that there's 1 with the flagging functionality (which i assume is in repoService
?) so we should keep the base and hte one that does the flagging functionality can just call super
.
idk if this is done here or downstream (cos i see that this is still unchanged) but if it's resolved downstream, feel free to resolve this
e56e758
to
a0ae4c9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm; please either update the diffed method here or downstream prior to merge
@seaerchin for clarity
this is already done a0ae4c9. may i confirm that this is what your meant by |
Merge activity
|
adding test cases for the two new functions `renameSinglePath` and `deleteDirectory` introduced into `GithubService` in #1042. Note that these functions already existed, but this pr adds the test cases to the right file.
Problem
Moving forward, since we have migrated most sites out of gh, we will only offer quickie for sites that are in ggs.
As such, this pr removes the dependency of a no longer supported flow so as to allow for a simpler mental model.
Additionally, there were weird abstractions bet GithubService + GithubCommitService -> removed the responsibilities to be isolated.
Test cases will be added in the leaf node of these prs, cicd will also be fixed upstream.