Refactor: split issue and pull request logic for improved clarity#499
Refactor: split issue and pull request logic for improved clarity#499TobyCyan wants to merge 26 commits intoCATcher-org:mainfrom
Conversation
SAN-MUYUN
left a comment
SAN-MUYUN
left a comment
There was a problem hiding this comment.
Thank you for refactoring such a large codebase, really appreciate the effort.
You may want to take a look at the suggestions regarding naming convention and some encapsulation issues.
| this.milestone = githubIssue.milestone ? new Milestone(githubIssue.milestone) : Milestone.PRWithoutMilestone; | ||
| } | ||
|
|
||
| public static createPhaseBugReportingPullRequest(githubIssue: GithubIssue): RepoItem { |
There was a problem hiding this comment.
this method createPhaseBugReportingPullRequest is taken from another project CATcher, which consists of a Bug Reporting Phase, hence the method name makes sense. Since this PR refactors the Issue class, will it be possible to change the name of this method to something more relevant to WATcher? Maybe createPullRequest will be better?
Same for the createPhaseBugReportingIssue above.
| } else if (this.repoItem instanceof PullRequest && this.repoItem.state === 'CLOSED') { | ||
| return 'border-red'; | ||
| } else if (this.issue.issueOrPr === 'Issue' && this.issue.stateReason === 'NOT_PLANNED') { | ||
| } else if (this.repoItem instanceof Issue && this.repoItem.stateReason === 'NOT_PLANNED') { |
There was a problem hiding this comment.
When checking for the type of repoItem, whether it is an Issue or PullRequest object, would it be better if you call the isPullRequest method provided below?
It would be even better if you can implement a method to get the type, or maybe adding a readonly field to each class would do job. Using instanceof to check for the type of repoItem somewhat defeats the purpose of refactoring the codebase in my opinion.
There was a problem hiding this comment.
I agree with your views, updated the PR 😸 !
| this.prCount = data.filter((issue) => issue.issueOrPr === 'PullRequest').length; | ||
| data = applySearchFilter(this.filter.title, this.displayedColumn, this.repoItemService, data); | ||
| this.issueCount = data.filter((datum) => datum instanceof Issue).length; | ||
| this.prCount = data.filter((datum) => datum instanceof PullRequest).length; |
…TobyCyan/WATcher into 464-refactor-split-issue-and-pr
3 participants
Summary:
Fixes #464
Type of change:
Changes Made:
IssueandPullRequestto extend from a base abstractRepoItemclass.Proposed Commit Message:
Checklist: