code-corps / code-corps-api

Elixir/Phoenix API for Code Corps.
https://www.codecorps.org
MIT License
234 stars 86 forks source link

Remove unused code, unnest modules #1367

Closed begedin closed 6 years ago

begedin commented 6 years ago

Problem

Through #1092, much of the code was unraveled and decoupled.

However, some of the intermediary functions in mid-level modules as well as their tests, were left in. We need to remove that code, unnest the modules and rewrite any tests that aren't redundant to call and test the higher level functions in the Sync module itself.

Subtasks

Notes

This PR might be large in scope, so as an alternative, It may be prudent to consider just the "identification" step part of this PR, and instead of doing the actual coding work here, create issues for each identified function/untested area.

joshsmith commented 6 years ago

I'm a little unclear as to how to proceed with this PR, so perhaps we should do that identification step first to make the next actions more direct.

joshsmith commented 6 years ago

Some quick documenting here:

CodeCorps.GitHub.Sync

GitHub syncing functions for:

  • events received from the GitHub API
  • entire GitHub repositories

Types

Functions

CodeCorps.GitHub.Sync.Comment

Missing moduledoc.

Functions

CodeCorps.GitHub.Sync.Comment.Comment

In charge of syncing CodeCorps.Comment records with a GitHub comment payload.

A single GitHub comment always matches a single CodeCorps.GithubComment, but it can match multiple CodeCorps.Comment records. This module handles creating or updating all those records.

Types

Functions

CodeCorps.GitHub.Sync.Comment.Comment.Changeset

In charge of building a Changeset to update a Comment with, when handling a GitHub Comment payload.

Functions

CodeCorps.GitHub.Sync.Comment.GithubComment

In charge of finding a CodeCorps.GithubComment to link with a CodeCorps.Comment when processing a GitHub Comment payload.

The only entry point is create_or_update_comment/2.

Types

Functions

CodeCorps.GitHub.Sync.Installation.Changeset

In charge of managing changesets when creating or updating a GithubAppInstallation in the process of handling an Installation event.

Functions

CodeCorps.GitHub.Sync.Issue

Functions

CodeCorps.GitHub.Sync.Issue.GithubIssue

In charge of finding or creating a CodeCorps.GithubIssue to link with a CodeCorps.Task when processing a GitHub Issue payload.

The only entry point is create_or_update_issue/2.

Types

Functions

CodeCorps.GitHub.Sync.Issue.Task

Missing moduledoc.

Types

Functions

CodeCorps.GitHub.Sync.Issue.Task.Changeset

In charge of building a Changeset to update a Task with, when handling an Issues webhook.

Functions

CodeCorps.GitHub.Sync.PullRequest

In charge of finding a pull request to link with a GithubPullRequest record when processing a GitHub Pull Request payload.

The only entry point is create_or_update_pull_request/2.

Functions

CodeCorps.GitHub.Sync.PullRequest.BodyParser

In charge of extracting ids from markdown content, paired to a predefined list of keywords.

Functions

CodeCorps.GitHub.Sync.User.RecordLinker

In charge of finding a user to link with a given record given a GitHub API payload containing the user.

The only entry point is link_to/2.

Types

Functions

joshsmith commented 6 years ago

The above means there are types of:

All to represent certain outcomes. We should try to standardize those.

joshsmith commented 6 years ago

In the various syncing modules I see the following exposed as public functions:

Again, there appears to be some inconsistencies here that could probably be cleaned up, condensed, and standardized somehow.

joshsmith commented 6 years ago

There are also inconsistencies in the moduledocs (and some missing moduledocs) that could use cleanup. Also not sure I like the wording of:

In charge of

Perhaps we should do some looking at how Elixir itself words its moduledocs for some stylistic convention seeking.

joshsmith commented 6 years ago

I'm probably missing something, but I'm having trouble identifying which intermediary functions are missing right now after the cleanup.

begedin commented 6 years ago

Just FYI, I'm working through these as part of your PR in #1370

begedin commented 6 years ago

We can considered this closed through #1370

Any remaining tests still required are covered by other issues