Conversation
|
This pull request can be reviewed with Preview Code. |
| githubService.setOrderingStatus(repoAndPull.second, pendingStatus); | ||
|
|
||
| //Add hunks to database | ||
| Diff diff = githubService.fetchDiff(repoAndPull.second); |
There was a problem hiding this comment.
Eventually, this should also be refactored into an Action<..> but for this PR it's fine
| return insertPullIfNotExists(pull) | ||
| .then(this::clearExistingGroups) | ||
| .then(dbPullId -> traverse(groups, createGroup(dbPullId))).toUnit(); | ||
| .then(dbPullId -> traverse(groups, createGroup(dbPullId, null))).toUnit(); |
There was a problem hiding this comment.
To avoid nulls floating in the code, I'd suggest we make this false and adjust DatabaseInterpreter.insertNewGroup to insert null if this parameter is false.
| * @param diff to parse | ||
| * @return list of hunkChecksums | ||
| */ | ||
| public List<String> parseDiff(String diff) { |
There was a problem hiding this comment.
Can the return type be List<HunkChecksum>? This is more precise and avoids the need for the Javadoc comment
| /** | ||
| * The hunkChecksums for the diff. | ||
| */ | ||
| private final List hunkChecksums; |
There was a problem hiding this comment.
This should be List<HunkChecksum/String>
| /** | ||
| * @return the hunkChecksums | ||
| */ | ||
| public List getHunkChecksums() { |
| } | ||
|
|
||
| @Override | ||
| public Action<Unit> insertDefaultGroup(PullRequestIdentifier pull, List<OrderingGroup> groups) { |
There was a problem hiding this comment.
This method always gets called with a list of one group. You can eliminate the list and just make the argument an OrderingGroup
| .on(InsertPullIfNotExists.class).returnA(pullRequestID) | ||
| .on(FetchGroupsForPull.class).returnA(List.empty()) | ||
| .on(NewGroup.class).apply(action -> { | ||
| assertThat(action.defaultGroup).isEqualTo(true); |
There was a problem hiding this comment.
This assertion is unnecessary.
| @Override | ||
| public Action<Unit> insertDefaultGroup(PullRequestIdentifier pull, List<OrderingGroup> groups) { | ||
| return insertPullIfNotExists(pull) | ||
| .then(this::clearExistingGroups) |
There was a problem hiding this comment.
This seems unnecessary and dangerous 😅
|
|
||
| Interpreter interpreter = | ||
| interpret() | ||
| .on(InsertPullIfNotExists.class).returnA(pullRequestID) |
There was a problem hiding this comment.
If you remove this line, the test will fail if the insertion of a default group depends on existing groups. This would catch the unnecessary clearExistingGroups action in DatabaseService.
| .on(AssignHunkToGroup.class).apply(toUnit(action -> { | ||
| assertThat(List.of(group).find(g -> g.id.equals(action.groupID))).isNotEmpty(); | ||
| Option<HunkChecksum> hunkID = hunkIDs.find(id -> id.checksum.equals(action.hunkChecksum)); | ||
| assertThat(hunkID).isNotEmpty(); |
There was a problem hiding this comment.
And while you're editing this file, please fix the indentation
* Added initial version of approvingAPI v2 * Allow users to set the approval status multiple times * Implement getApprovals * Implement and test GitHub authentication actions (#53) * Add tests for getApprovals actions * Added two tests (then stepper was empty) * Test that evaluation of getApproval finishes * Added enpoint for fetching hunk approvals * Added tests for getHunkApprovals * Added javadoc for approvalsAPI * Change all occurrences of Seq to List * Implement DeleteGroup in DatabaseInterpreter * Ensure ApproveStatus is never null * Implement FetchHunksForGroup in DatabaseInterpreter * Make Approvals refer to a hunk by their ID * Constrain hunks to be unique per pull request * Coerce the jOOQ api into handling 'INSERT INTO ... RETURNING ...' queries * Change the database dsl to return entire hunks/approvals * Fix tests for database service * Re-implement hunk and approval fetching * Implement generic deserialization of types with one field * Implement wrapped type serialization * Test wrapped type de/serialization * Implement github authentication actions and a caching interpreter * Import hunks when a PR is opened (#54)
2 participants
No description provided.