smoke test passes, and yes, I''m also running Checkstyle.
http://gwt-code-reviews.appspot.com/338801/diff/1/9 File user/test/com/google/gwt/i18n/shared/AnyRtlDirectionEstimatorTest.java (right): http://gwt-code-reviews.appspot.com/338801/diff/1/9#newcode25 user/test/com/google/gwt/i18n/shared/AnyRtlDirectionEstimatorTest.java:25: private static AnyRtlDirectionEstimator estimator = On 2010/04/11 19:50:19, jat wrote:
On 2010/04/11 19:16:50, Tomer wrote: > On 2010/04/11 16:13:46, jat wrote: > > Would it be worth moving estimator to DirectionEstimatorTestBase
(set in a
> super > > constructor call) and make each individual test just a set of
expected
results > > for various strings? > > I don't realize what it would save - we still should have all types
of
> estimators, one for each subclass. On the other hand, it'd impair
readability.
I was just thinking there is a lot of duplication among the different
tests, and
it could be slightly reduced by doing something like:
assertEstimatedDirection(Direction.RTL, containsRtlChar); assertEstimatedDirectionHtml(Direction.LTR, noRtlCharsHtml);
in the test methods, with assertEstimatedDirection* defined on the
base class
and estimator initialized there from a superclass constructor. To me,
that
seems more readable if the duplicated code is reduced.
However, this isn't a big deal so if you feel strongly about it feel
free to
leave as-is.
Sorry, I didn't fully understand you earlier. This is indeed a more elegant design. http://gwt-code-reviews.appspot.com/338801/diff/9001/10005 File user/src/com/google/gwt/i18n/shared/HasDirectionEstimator.java (right): http://gwt-code-reviews.appspot.com/338801/diff/9001/10005#newcode36 user/src/com/google/gwt/i18n/shared/HasDirectionEstimator.java:36: * DirectionEstimator object to {...@link DefaultDirectionEstimator}. On 2010/04/11 19:50:19, jat wrote:
In light of your comments about how different estimators will be used,
is it
appropriate to explicitly mention DefaultDirectionEstimator here? It
seems like
some widgets might have different defaults.
Perhaps it would be better to say "...to a default DirectionEstimator
instance". Makes sense. I also omitted the defaultDirectionEstimator, and instead added a note in WordCoundDirectionEstimator stating that it is recommended for most use cases. http://gwt-code-reviews.appspot.com/338801/diff/9001/10008 File user/test/com/google/gwt/i18n/shared/AnyRtlDirectionEstimatorTest.java (right): http://gwt-code-reviews.appspot.com/338801/diff/9001/10008#newcode25 user/test/com/google/gwt/i18n/shared/AnyRtlDirectionEstimatorTest.java:25: private static final AnyRtlDirectionEstimator ESTIMATOR = On 2010/04/11 19:50:19, jat wrote:
Sorry, I should have been more clear.
Constants should be all-caps and underscore separated, but that
doesn't mean
that everything immutable is considered a constant. INSTANCE for
singleton
instances is somewhat of a special case, and it can be fuzzy on where
to draw
the line. Personally, I use all-caps for Enums, Strings, and
primitives.
So, I think estimator is fine here.
Is there a more accurate definition? Anyway, it doesn't exist anymore. I'm changing INSTANCE in the various estimators as well. http://gwt-code-reviews.appspot.com/338801/diff/9001/10008#newcode34 user/test/com/google/gwt/i18n/shared/AnyRtlDirectionEstimatorTest.java:34: assertEquals(ESTIMATOR.estimateDirection(containsRtlChar), Direction.RTL); On 2010/04/11 19:50:19, jat wrote:
Sorry I didn't notice this before -- you have expeced/actual backwards
on the
asserts.
Done. http://gwt-code-reviews.appspot.com/338801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
