search

mockito / shipkit

Toolkit for shipping it used by Mockito library
http://shipkit.org
MIT License
158 stars 35 forks source link

refactored UpdateReleaseNotes #613

Closed epeee closed 6 years ago

epeee commented 6 years ago

@wwilk a very first version of the refactoring we were talking about in one of the last pr's.

Javadoc is still missing, also the naming of certain classes might change and also visibility. I just wanted to get your feedback. To sum it up, it is more code now but it is also more separated and reusable.

wwilk commented 6 years ago

I think complexity might be increased in general. On the other hand at the high level in UpdateReleaseNotes it was decreased due to usage of builder. So I like the fact that this complexity was removed from UpdateReleaseNotes, but that's true that it's a lot of code for a simple thing, and not really a lot of this code we can reuse. So I'm neutral in this case, whatever you guys decide.

epeee commented 6 years ago

Yes, complexity increased a little bit due to the builder. On the other hand this abstraction allows us to decouple the map generation in a clean way from the UpdateReleaseNotes. The builder also generates ~half of the new lines of code. I'm ok with not merging it, I just thought it would make UpdateReleaseNotes a little bit cleaner/separate certain logic and therefore make it easier to test.

mockitoguy commented 6 years ago

@mockito/shipkit-developers, we need one more opinion, who is going to be a tie-breaker?

If there is no other opinion, I will consider my vote as +1.01 and close the PR without merging. Good discussion!

epeee commented 6 years ago

yep, let's close this one and go for less code.