[
https://issues.apache.org/jira/browse/SHINDIG-262?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12596424#action_12596424
]
Cassie Doll commented on SHINDIG-262:
-------------------------------------
thanks for the patch!
there were several things i had to fix or change in your patch
style issues:
- watch your indents. line continuations should be indented by 4 spaces
- all lines must be less than 80 chars
- you had some double spaces between some vars like {RIGHT,<space><space>
START_EDGE} which should be just single spaces
- the substitution field had a spelling typo
code changes:
- i noticed the addSubstitutions line was being repeated so i moved that into
the helper method
- i changed from using a String[][] which seemed easy to mix up to using 4
String variables with two more helper vars. this makes the tests a little
easier to read.
- your last two tests were failing (w/null and empty string) because the
default behavior is ltr not rtl
- you forgot super.setUp() in your overridden method, but i inlined the var
away anyway
now that you are aware of some of our code practices i'm sure there will be
less revisions to a patch next time.
thanks again for your help!
- cassie
> Added Tests for BidiSubstituter
> -------------------------------
>
> Key: SHINDIG-262
> URL: https://issues.apache.org/jira/browse/SHINDIG-262
> Project: Shindig
> Issue Type: Improvement
> Components: Common Components (Java)
> Environment: Place in test folder in org.apache.shindig.gadgets
> package
> Reporter: Ralph Jocham
> Attachments: bidi-substituter-test.patch
>
>
> /**
> * Licensed to the Apache Software Foundation (ASF) under one
> * or more contributor license agreements. See the NOTICE file
> * distributed with this work for additional information
> * regarding copyright ownership. The ASF licenses this file
> * to you under the Apache License, Version 2.0 (the
> * "License"); you may not use this file except in compliance
> * with the License. You may obtain a copy of the License at
> *
> * http://www.apache.org/licenses/LICENSE-2.0
> *
> * Unless required by applicable law or agreed to in writing,
> * software distributed under the License is distributed on an
> * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
> * KIND, either express or implied. See the License for the
> * specific language governing permissions and limitations under the License.
> */
> package org.apache.shindig.gadgets;
> import junit.framework.TestCase;
> public class BidiSubstituterTest extends TestCase {
> private static final String REVERSE_DIR = "REVERSE_DIR";
> private static final String START_EDGE = "START_EDGE";
> private static final String END_EDGE = "END_EDGE";
> private static final String DIR = "DIR";
> private static final String LTR = "ltr";
> private static final String RIGHT = "right";
> private static final String LEFT = "left";
> private static final String RTL = "rtl";
> private Substitutions substitions;
> @Override
> public void setUp() {
> substitions = new Substitutions();
> }
> public void testBidiWithRtl() {
> BidiSubstituter.addSubstitutions(substitions, RTL);
> assertSubstitutions(substitions,
> new String[][] {{LTR, REVERSE_DIR}, {RIGHT,
> START_EDGE},
> {LEFT, END_EDGE}, {RTL, DIR}});
> }
> public void testBidiWithLtr() {
> BidiSubstituter.addSubstitutions(substitions, LTR);
> assertSubstitutions(substitions,
> new String[][] {{RTL, REVERSE_DIR}, {LEFT,
> START_EDGE},
> {RIGHT, END_EDGE}, {LTR, DIR}});
> }
> public void testBidiWithEmpty() {
> BidiSubstituter.addSubstitutions(substitions, "");
> assertSubstitutions(substitions,
> new String[][] {{LTR, REVERSE_DIR}, {RIGHT,
> START_EDGE},
> {LEFT, END_EDGE}, {RTL, DIR}});
> }
> public void testBidiWithNull() {
> BidiSubstituter.addSubstitutions(substitions, null);
> assertSubstitutions(substitions,
> new String[][] {{LTR, REVERSE_DIR}, {RIGHT,
> START_EDGE},
> {LEFT, END_EDGE}, {RTL, DIR}});
> }
> private void assertSubstitutions(Substitutions substitutions, String[][]
> mappings) {
> for (String[] mapping : mappings) {
> assertEquals(mapping[0],
> substitutions.getSubstitution(Substitutions.Type.BIDI, mapping[1]));
> }
> }
> }
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
