Closed GoogleCodeExporter closed 8 years ago
Hi there, I am excited that you're interested in reusing plovr, so I don't want
to dampen your enthusiasm, but I don't agree with all of your suggestions.
For example, it is extremely helpful that ConfigOption depends on Gson. By
construction, it ensures that each option can be parsed from a config file.
This is later leveraged by ConfigOptionDocumentationGenerator, which makes it
possible to autogenerate http://plovr.com/options.html, which ensures that the
documentation is in sync with the code.
If you are going to use a Groovy DSL, then I don't see why ConfigOption's
dependency on Gson should even concern you as you should be using Config
directly. Though I'm not sure how you would create one since Config.Builder is
deliberately package-private...As you can see, I am trying to be conservative
in plovr's API at this stage, meaning I am trying to expose as little of the
API as possible so that I don't have to worry about breaking people when I make
changes.
Further, the benefit of using a strongly typed language such as Java is that
you can use descriptive types to represent abstractions. If I used a Map
instead of QueryData, then everywhere I passed that Map around, I would also
need a comment (or variable name) to indicate that it represents query data.
Further, a Map is not a robust abstraction for query data because (1) a Map can
also have null keys and values which query data should not, and (2) query data
can have multiple values for the same key, which is a concept that Map does not
support.
If you believe strongly in this refactoring, then I would be happy to look at a
proposed patch. However, my priority is maintaining tight specifications and
strong invariants in the plovr codebase to reduce the possibility of
introducing bugs as I move forward. Creating helper classes with tight
specifications and leveraging the type-checking of the Java compiler are two of
the primary ways I help achieve this.
Original comment by bolinf...@gmail.com
on 2 Nov 2010 at 2:38
Ok, I can live with not using ConfigOption. But I would have to create my own
builder, so it would be useful if Config had some public setters or a
constructor. Since Config is a model class that is used throughout the
compilation core, it will be hard to reuse this without being able to construct
Config objects.
Original comment by mar...@better.se
on 2 Nov 2010 at 4:01
Would making Config.Builder public be sufficient? As you can see, I have gone
to great lengths to ensure that Config is immutable, so I want to be able to
restrict the ways in which it is constructed.
Original comment by bolinf...@gmail.com
on 2 Nov 2010 at 4:03
Yes, it looks like it would suffice. I'm not sure why Config needs to be
immutable *and* be without a public constructor, but if that's the way it is...
:)
Original comment by mar...@better.se
on 2 Nov 2010 at 4:21
I was able to do everything I need after making Config.Builder public. A
remaining quirk is Builder.addDefine which takes a JsonPrimitive. Can it take
an Object instead?
Original comment by mar...@better.se
on 8 Nov 2010 at 4:40
Hi Marcus, I have thought about this a little, and my concern is that changing
it to accept Object would be kinda gross. I would then have to do all sorts of
casting and error handling (for bad casts) inside my code.
Alternatively, I could create a new object system to handle JSON, but then I'd
probably end up with something analogous to the Gson API, anyway. And really,
the Gson API is much, much better than the "standard" one:
http://www.json.org/java/.
Looking at http://code.google.com/p/plovr/source/browse/#hg/lib, gson-1.4.jar
is only 170KB and its API is fairly small, so would it really be that bad to
add it as a dependency for your project?
Original comment by bolinf...@gmail.com
on 9 Nov 2010 at 3:32
I suggest to add overloaded (or just different) methods
Config.Builder.addDefine() with different types for the value, and move the
JsonPrimitive unwrapping to the ConfigOption. That should solve it nicely.
Gson as a dependency requires carrying an extra JSON implementation for
essentially only the "defines" option. I agree that 170 kB is not a big problem
in a webapp, but it's one more bit of bloat.
Original comment by mar...@better.se
on 9 Nov 2010 at 5:33
Hi Marcus, I went ahead and made Config.Builder public in
http://code.google.com/p/plovr/source/detail?r=7ee3a4bf14e4d1bd8f559b60280592f08
7faa62d.
I took another look at addDefine(), but I really don't want to change it right
now because that means that I have to do one of:
(1) Add three methods addDefine(boolean), addDefine(number), addDefine(String)
and store three Map fields, one for each type.
(2) Add the three methods but store one Map field, but then end up with
something like the nasty getDefineReplacements() in
com.google.javascript.jscomp.CompilerOptions.
(3) Add one generic addDefine(Object) method and do a bunch of nasty casting.
In the future, I would like to make it possible to do _better_ than the Closure
Compiler API and be able to take _any_ JSON object in the plovr config file and
use it as the value of a define. That means that ultimately the signature of
this method will be:
addDefine(String name, com.google.gson.JsonObject anything)
At that point, I really don't want to have addDefine(String name, Object
anything) because the implementation would be even nastier -- using the Gson
object model is really the way to go, so since it's going to be the future, I'm
still going to leave it in now.
I think that accepting Gson as a dependency of plovr (rather than fighting it)
will likely make it easier to do other JSON-y things with plovr down the road.
For example, it would have also been possible to implement plovr without Guava,
or to try to limit its use, but it's a good API and is extremely helpful, so
there's no reason to limit its use.
At this point, I am going to mark this issue closed. I hope we can just agree
to disagree on this one.
Original comment by bolinf...@gmail.com
on 2 Dec 2010 at 3:10
Original issue reported on code.google.com by
mar...@better.se
on 29 Oct 2010 at 12:55