Closed begedin closed 6 years ago
@begedin I see you added some commits after your needs code review
label. Is this awaiting review for sure?
It also looks like there are test failures with a coverage drop.
Found more stuff to cleanup here. Basically, I've decoupled and flattened out the sync module structure.
I'll be able to remove some of the code which is unused now, as well as moving some tests around.
The line count in the sync module is higher now, but the benefit is that we have a much clearer picture on how to break things down into the queue system, for example.
We also have a much clearer picture of what's tested and what's untested. One look at the coverals report should explain that.
Basically, we were unit testing bits, but not all integrated paths are covered properly.
I can continue work by cleaning up unused code and moving tests around/adding tests as part of this PR next week, or we can merge this one and do it separately. It should not be put on the backlog, though, since there will be chunks of unused code in the codebase now, which are bound cause confusion and wasted time down the line.
Created #1367
What's in this PR?
Performs some long needed cleanup for some of our Github modules. While this does not tackle everything listed under #1092, I added a comment into the issue where I'm explaining why we might not want to do all of it.
Done - Changesets
create_changeset
andupdate_changeset
functionAlso done
Moved
InstallationRepositories
event logic into theSync
namespace MovedInstallation
event logic into theSync
namespaceReferences
Progress on: #1092
Notes
The
Sync
module is moving towards unmanageable. It seems like we're just shifting the weight from one end to the other, instead of distributing it.Couple of things I suggest
Split up the
Sync
context intoSync
andEvent
. We don't need individual event modules, just a singleEvent
module with all thex_event
functions. That being said, I'd wait with this, since everything might change with adding the queue.Split out the single
outcome
type intox_outcome
for each method. While there are some overlaps, the types our mostly different, so it's just hard to keep track when we push them into a single type.Same for
marshall_response
.I already did both for
installation_event/1
. It makes it much cleaner, IMO. The output and output type are kept closer together and do not touch on any other.