search

evanchueng / gerrit

Automatically exported from code.google.com/p/gerrit
Apache License 2.0
0 stars 0 forks source link

Refactor code duplication in AccountDiffPreference, PatchScriptSettings and PrettySettings #629

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago 
Persisting of diff preferences was done in this issue:
http://code.google.com/p/gerrit/issues/detail?id=579

Since there was some code duplication in the
AccountDiffPreference vs (PatchScriptSettings + PrettySettings)
it was agreed to refactor it in a follow up issue.

Original issue reported on code.google.com by ziv...@gmail.com on 19 Jul 2010 at 9:06

GoogleCodeExporter commented 9 years ago 
It looks like this refactoring will require a change in project dependencies.
Here is an overview of the projects where the classes AccountDiffPreference,
PatchScriptSettings and PrettySettings belong and their dependencies:

Project         Class
gerrit-common   PatchScriptSettings
gerrit-prettify PrettySettings
gerrit-reviewdb AccountDiffPreference

Project         Dependends on
gerrit-common   gerrit-prettify
gerrit-common   gerrit-reviewdb
gerrit-reviewdb gwtorm

As far as I understand it, the AccountDiffPreference class must be there in the
reviewdb project in order to be persisted (because this is where we have
reference to gwtorm and can use @Column, @Relation and other annotations).
If we would like keep the AccountDiffPreference and remove the PrettySettings
and PatchScriptSettings then the problem would be that there is no dependency
from gerrit-prettify to gerrit-reviewdb and this dependency would have to
be introduced.

Any comments, opinions?

Original comment by ziv...@gmail.com on 19 Jul 2010 at 9:26

GoogleCodeExporter commented 9 years ago 
It should be OK to add gerrit-reviewdb as a dependency on
the gerrit-prettify component.  The only reason its not there
is because we didn't need anything.  By adding the new
dependency to the project we should be able to reduce some
code, and make the system a little easier to follow, so its
well worth the dependency change.

Original comment by sop@google.com on 19 Jul 2010 at 2:04

GoogleCodeExporter commented 9 years ago 
Fixed with commit: 8e33d76853200f922b170aed7412761cd5c16dbb

Original comment by ziv...@gmail.com on 22 Jul 2010 at 11:19

GoogleCodeExporter commented 9 years ago

Original comment by sop@google.com on 28 Mar 2012 at 2:55