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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Add coverage to Subject.sync #1428

wants to merge 1 commit into
base: master


None yet
2 participants
Copy link

AndyStabler commented Jan 5, 2019

馃憢 I've got my eye onSubject.sync for some refactoring, but first I thought I'd add some tests to make sure I don't break things going forward.

I haven't implemented all the tests just yet because I thought you might have opinions on how they're structured, or you might not be interested in this change (which is totally fine).

My thinking is this:

  1. Add code coverage in this branch
  2. In a new branch, extract an instance method Subject#sync since most of the things going on in Subject.sync are working on an instance anyway.
  3. Maybe extract some private method while I'm on (e.g. extract_full_name_from_remote_subject)

Once that's in place, we might be in a better position to do some larger refactoring of the class depending on how you feel.

The class is fairly generic, but seems to have logic in there that's specific to certain kinds of subjects (knowledge of whether the subject is merged, or if a subject is a commit and has comments), which mixes the layers of abstraction a bit. We could introduce a template method pattern where the shared/default behaviour lives in the base class Subject, and the sub classes of Subject (Commit, PullRequest, etc) override certain methods like full_name or comment_count if they need to. That could all be overkill though and I don't have too much domain knowledge here, so I'm keen to know what you think!


This comment has been minimized.

Copy link

andrew commented Jan 5, 2019

@AndyStabler I like where this is going 馃憤

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment