Project

General

Profile

Feature #1591

implement QUOTER built-in function

Added by Greg Shah over 11 years ago. Updated over 7 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Costin Savin
Start date:
10/15/2012
Due date:
% Done:

100%

Estimated time:
16.00 h
billable:
No
vendor_id:
GCD

cs_upd20121022a.zip (46.1 KB) Costin Savin, 10/23/2012 05:31 AM

cs_upd20121121a.zip (46.3 KB) Costin Savin, 11/21/2012 07:52 AM

cs_upd20121121d.zip (52.6 KB) Costin Savin, 11/21/2012 02:12 PM

cs_upd20121127a.zip (46.3 KB) Costin Savin, 11/27/2012 08:56 AM

cs_upd20121214b.zip (46.4 KB) Costin Savin, 12/14/2012 07:16 AM

cs_upd20121217b.zip (46.4 KB) Costin Savin, 12/17/2012 02:30 PM

cs_upd20121217c.zip (46.4 KB) Costin Savin, 12/17/2012 02:39 PM

cs_upd20121218a.zip (46.4 KB) Costin Savin, 12/18/2012 12:27 PM

cs_upd20121221a.zip (46.6 KB) Costin Savin, 12/21/2012 01:09 PM

cs_upd20121231a.zip (40 KB) Costin Savin, 12/31/2012 06:41 AM

History

#1 Updated by Greg Shah over 11 years ago

Look at the QUOTER built-in function in the Progress 4GL Language Reference (it is in v10.x or later). Your task is to create a set of static methods in the com/goldencode/p2j/util/character.java class to implement this behavior. Please look at other sets of methods like indexOf() or lookup() to get the idea of how to implement the code. The reason you need to use an overloaded set of methods is because there are optional parameters AND some parameters can have both literal forms (String or int) as well as the BaseDataType "wrapper" forms like character or integer. We have to have the full set of possibilities accounted for.

Usually, all of them will resolve down to a single method that is the real worker.

Then you will need to add conversion support for this function. You will find that easy to do in rules/convert/builtin_functions.rules.

Write some testcases to try it out. Hopefully the 4GL dev system access will be available in time to allow you to compare your implementation with the 4GL.

#2 Updated by Costin Savin over 11 years ago

  • Status changed from New to WIP

#3 Updated by Costin Savin over 11 years ago

QUOTER utility specifications:
The QUOTER utility formats character data in a file to the standard format so it can be
used by an ABL (Advanced Business Language) procedure. By default, QUOTER does
the following:

  • Takes the name of a file as an argument
  • Reads input from that file
  • Places quotes (“) at the beginning and end of each line in the file
  • Replaces any already existing quotes (“) in the data with two quotes (“ ”)

You can use the QUOTER utility directly from the operating system using the operating
system statement, as appropriate, to reformat data from a file that is not in the standard
ABL input format:

quoter input-file-name
[   >output-file-name

 | -d character

 | -c startcol-stopcol

]

input-file-name
Specifies the file you are using.
> output-file-name
Specifies the file where you want to send the output.
-d character
Identifies a field delimiter. You can specify any punctuation character.
-c startcol-stopcol
Specifies the ranges of column numbers to be read as fields. Do not use any
spaces in the range list.

Examples:
OS-COMMAND SILENT quoter i-datfl3.d > i-datfl3.q.

OS-COMMAND SILENT quoter -d , i-datfl4.d >i-datfl4.q.

OS-COMMAND SILENT quoter -c 1-2,4-20,22-24 i-datfl5.d >i-datfl5.q.

#4 Updated by Costin Savin over 11 years ago

Quoter function
Converts the specified data type to CHARACTER and encloses the results in quotes
when necessary.
The QUOTER function is intended for use in QUERY-PREPARE where a character
predicate must be created from a concatenated list of string variables to form a WHERE
clause. In order to process variables, screen values, and input values so that they are
suitable for a query WHERE clause, it is often necessary to enclose them in quotes.
For example, European-format decimals and character variables must always be
enclosed in quotes.
Syntax:
QUOTER ( expression [ , quote-char [ , null-string ] ] )

Example:

DEFINE VARIABLE mychar As CHARACTER NO-UNDO INITIAL "Lift Line Skiing".
...
qhandle:QUERY-PREPARE("FOR EACH Customer WHERE Customer.Name = " +
QUOTER(mychar))

It seems this is the required function to implement.

#5 Updated by Greg Shah over 11 years ago

Yes, please implement the QUOTER built-in function NOT the QUOTER utility.

#6 Updated by Costin Savin over 11 years ago

  • File cs_upd_20121022.zip added

Added proposed update for quoter builtin function

#7 Updated by Costin Savin over 11 years ago

  • File deleted (cs_upd_20121022.zip)

#8 Updated by Costin Savin over 11 years ago

#9 Updated by Greg Shah over 11 years ago

  • Target version set to Milestone 7

#10 Updated by Greg Shah over 11 years ago

Feedback on the proposed changes:

1. Each file with a standard header needs the copyright notice updated to 2012 if it hasn't already been updated.

2. Each file with a standard header needs a history entry added to describe your change.

3. Don't import specific classes unless it is a requirement (this is only needed to resolve conflicts where the same class name appears in multiple packages).

4. Please do not use hard tabs. Configure your editor to convert the tab key into 3 spaces so that there are no tab characters (0x09) in our source code.

5. Each line, including JavaDoc lines must be limited to 78 characters in length.

6. Please reformat the JavaDoc to conform to the same look (including extra blank lines and the horizontal spacing) as the other methods in that same file. They should already be following the standards.

7. In quoter(character), move the in and out strings to be static final String constants (as members of the class). Such things as unnecessary string concatenation should not be done more than once if not needed.

8. It seems that quoter(character) could be implemented like this:

return charVar.isUnknown() ? new character()
: concat('"', charVar.value.replaceAll(in, out), '"');

9. It seems that quoter(BaseDataType) could be implemented like this:

return var.isUnknown() ? new character() : concat('"', var, '"');

10. The code is missing support for a custom quote character and for a null string override. This means a you would create a worker (instead of the approach in #8 and #9 above) that has a signature like this:

private character quoterWorker(BaseDataType var, char quote, String nullTxt) {
// you can even detect the standard BaseDataType versus the character type right here
if (var instanceof character) {
// extra doubling of " chars here
}

// standard processing
}

Then add all the overloaded methods needed to support the possible calls (with and without optional parms). Each overloaded method would just call the worker to get its job done.

Make sure you have 4GL testcases that fully explore these various options.

11. Don't name all your testcases starting with the word "Test" or "test". Just make a descriptive name.

12. Don't use uppercase in testcase filenames and don't camel-case. In Progress, the convention is to use a "-" character to separate syllables. Our default name conversion will give good results for this.

13. Please merge your changes into the latest version of rules/convert/builtin_functions.rules since CVS has a newer version now.

#11 Updated by Costin Savin over 11 years ago

  • Status changed from WIP to Review

#12 Updated by Costin Savin over 11 years ago

  • Status changed from Review to WIP

#13 Updated by Costin Savin over 11 years ago

There seems to be a problem for date variables
Assuming we have the following variable:

def var t-date1  as date format '99/99/9999' init 01/01/2033.

The string equivalent of the date variable will be 01/01/33 in p2j( the format doesn't seem to be taken into consideration when creating a the new date var, shouldn't the date class also include the format when created?)
In Progress the string value used by quoter for this date variable is 01/01/2033 (as the format implies) this will produce different quoter values in p2j and Progress ("01/01/33" vs "01/01/2033").

#14 Updated by Greg Shah over 11 years ago

Please provide a small sample here that shows the problem. Please note that when you say "string equivalent of the date variable", this actually means different things in Progress, depending on how that string is generated. It is not as simple as it seems on the surface.

#15 Updated by Costin Savin over 11 years ago

This seems to be only reproducible for quoter

def var t-date1  as date format '99/99/9999' init 01/01/2033.
message quoter(t-date1)

Tried to reproduce it with
def var t-date1  as date format '99/99/9999' init 01/01/2033.
message t-date1.

this will output 01/01/33 in both p2j and Progress

and also

def var t-date1  as date format '99/99/9999' init 01/01/2033.
display t-date1.

this will output 01/01/2033 in both p2j and Progress.

The difference between the 2 is that display takes the widget which also contains the formatting, while message takes the date variable.
Quoter in progress seems to be using the widget-like value as input which also contains the formatting.

#16 Updated by Greg Shah over 11 years ago

This means that during conversion, you must emit any non-default format string for the variable into the quoter() call. For example:

quoter(t-date1).

becomes:

quoter(tDate1, <format_string>);

Then you can honor the format string as needed inside quoter().

You can access the format string from the variable's definition, which can be accessed from the refid annotation of the VAR_DATE instance.

#17 Updated by Costin Savin over 11 years ago

That may pose some problems since quoter can also be called with 2 or 3 parameters (custom unknown replacement and custom quote replacement both which can be strings).
Will look into this

#18 Updated by Greg Shah over 11 years ago

Good point. In this case, it is probably best to call a variant of quoter(). Perhaps it should be named quoterFormatted(). That version would always have a 2nd String parm that is the explicit format to honor. Then it can have optional 3rd/4th args for the custom unknown replacement and custom quote replacement Strings.

#19 Updated by Greg Shah over 11 years ago

Make sure you test each data type with explicit format strings that are different from the default.

Example, try "x(3)" for character data that is longer that 3 characters ("xxxxxxxxxxxxxxxxxxxx").

#20 Updated by Costin Savin over 11 years ago

After more tests it seems that in fact quoter uses a fixed format for date not the format it is defined with:

def var t-date1  as date format '99/99/99' init 01/01/2033.
display t-date1. /*output 01/01/33*/
message t-date1 /*output 01/01/33*/
message quoter tdate1. /*"01/01/2033"*/

This means that no rules have to be changed to call quoter with formatting (good since the rule change went well until you get a quoter with unknown literal case).
Format will have to be tested for other cases to determine default behaviour

#21 Updated by Costin Savin over 11 years ago

Is the main purpose of quoter to be used with database query instructions like where clauses?
If so, there might be some problems because of database platform specifics for example if we have the query:

def var mychar as character initial "John Deere".
def var my-query as character. 
my-query  = "FOR EACH Customer WHERE Customer.Name = " + QUOTER(mychar).
...

this will result in:
FOR EACH Customer WHERE Customer.Name = "John Deere"
but in order to work in let's say PostgreSQL the query will have to use ' like this:
FOR EACH Customer WHERE Customer.Name = 'John Deere'
The default null values can bring the same problem.

If the role of quoter is to be used like this it can have other implications and the implementation of quoter might have to be changed for defaults to meet database specifics.

#22 Updated by Greg Shah over 11 years ago

I understand. Don't worry about the fact that the result will not work in PostgreSQL. We know this. But any such database code that is created at runtime using QUOTER would have to be used with dynamic database features in the 4GL. In other words, those database where clauses and so forth will have to be converted from 4GL to Java at runtime. This is already planned as part of other tasks.

You only need to worry about exactly duplicating the behavior of QUOTER.

#23 Updated by Costin Savin over 11 years ago

After retest on Progress found that custom null can only be provided with 3 argument call, if a 2 argument it's tried quoter(var, customNull) it will not figure out that customNull parameter is not one of the two possible quote chars ' and " and will quote the variable with the first letter of the customNull variable if not null despite documentation stating that you can only give " or ' as custom quote characters.
Modified to fit behaviour of Progress.

#24 Updated by Costin Savin over 11 years ago

Added propposed update. Submited testcase quoter-builtin-function.p to bazaar testcases repository.

#25 Updated by Greg Shah over 11 years ago

Feedback:

1. Rename workQuoter() to quoterWorker().

2. workQuoter() is private static. Per the coding standards, we group all private static methods together. Move it to the proper location.

3. Please put spaces in before parenthesis in text. For example:

    * doubled(another double quote will be added next to it(Progress
    * behaviour).

should be this:

    * doubled (another double quote will be added next to it - Progress
    * behaviour).

Note that I also removed the extra unmatched open ( character.

4. When you reference null in JavaDoc, please do it as null.

5. When writing a comment, per the coding standards, please put spaces in between the comment characters and the text. There are multiple examples of this problem that need to be fixed. Here is one:

         //weird Progress behaviour if given custom character that 
         //isn't " or ' it will take the first letter as the custom quote char

Which should be this:

         // weird Progress behaviour if given custom character that 
         // isn't " or ' it will take the first letter as the custom quote char

By the way, the reason for this behavior actually makes sense (although many things that Progress does do NOT make sense). The idea is that this is supposed to be a quote character (a single character). This is not a quote string. Progress has no "single character" type like char in Java. So they just let you pass a string and then they only ever use the first char in the string.

6. Instead of this:

      String nullVal = "?";

      ...

      if (customNull != null)
      { // custom null character
         nullVal = customNull;
      }

Please code it in a more compact way, like this:

      String nullVal = (customNull != null) ? customNull : "?";

7. In regards to the customNull parameter, does Progress allow this to be more than 1 char in size? In other words, will Progress emit a multi-char string instead of the question mark?

8. The following should be made more compact:

      String quoteVal = "\"";
      if (customQuoter != null)
      {
         // custom quoter character
         //weird Progress behaviour if given custom character that 
         //isn't " or ' it will take the first letter as the custom quote char
         quoteVal = customQuoter.substring(0, 1);
      }

should be:

      String quoteVal = (customQuoter != null) ? customQuoter.substring(0, 1) : "\"";

9. Please do not put comments on the same line as {} characters. This:

   { // custom null character

should be this:

   { 
      // custom null character

10. This comment is slightly wrong (missing a 9 at the end) and it is missing the space up front. Change it from:

         //date format result of quoter is 99/99/999

to this:

         // date format result of quoter is 99/99/9999

11. Your placing of whitespace (or sometimes the lack of whitespace) needs improvement.

      // default values
      String nullVal = "?";
      String quoteVal = "\"";
      if (customQuoter != null)
      {
         // custom quoter character
         //weird Progress behaviour if given custom character that 
         //isn't " or ' it will take the first letter as the custom quote char
         quoteVal = customQuoter.substring(0, 1);
      }
      if (customNull != null)
      { // custom null character
         nullVal = customNull;
      }
      // for character values that contain the quote character
      // it will be doubled where met inside the text
      String in = quoteVal;
      String out = quoteVal + quoteVal;
      String result = nullVal;
      if (!var.isUnknown())
      {
         if (var instanceof character)
         {
            character charVar = (character) var;
            result = concat(quoteVal,
                            charVar.value.replaceAll(in, out),
                            quoteVal).value;
         }
         else if (var instanceof date)
         {
            date dateVar = (date) var;
            //date format result of quoter is 99/99/999
            result = concat(quoteVal,
                            dateVar.toString("99/99/9999"),
                            quoteVal).value;
         }
         else
         {
            //the rest of the data type format result of quoter
            //match the default message format (integer, decimal, logical etc)
            result = concat(quoteVal, 
                            var.toStringMessage(), 
                            quoteVal).value;
         }
      }
      return new character(result);

   }

I would space it like this:

      // default values
      String nullVal = "?";
      String quoteVal = "\"";

      if (customQuoter != null)
      {
         // custom quoter character
         //weird Progress behaviour if given custom character that 
         //isn't " or ' it will take the first letter as the custom quote char
         quoteVal = customQuoter.substring(0, 1);
      }

      if (customNull != null)
      { // custom null character
         nullVal = customNull;
      }

      // for character values that contain the quote character
      // it will be doubled where met inside the text
      String in = quoteVal;
      String out = quoteVal + quoteVal;
      String result = nullVal;

      if (!var.isUnknown())
      {
         if (var instanceof character)
         {
            character charVar = (character) var;
            result = concat(quoteVal,
                            charVar.value.replaceAll(in, out),
                            quoteVal).value;
         }
         else if (var instanceof date)
         {
            date dateVar = (date) var;
            //date format result of quoter is 99/99/999
            result = concat(quoteVal,
                            dateVar.toString("99/99/9999"),
                            quoteVal).value;
         }
         else
         {
            //the rest of the data type format result of quoter
            //match the default message format (integer, decimal, logical etc)
            result = concat(quoteVal, 
                            var.toStringMessage(), 
                            quoteVal).value;
         }
      }

      return new character(result);
   }

I believe the result is more readable this way. It also properly matches our coding standards.

12. Please put a space in between parameters to a method call. This:

return workQuoter(var,null,null);

should be this:

return workQuoter(var, null, null);

13. The JavaDoc here needs to be formatted properly. There is also a missing blank line between the end of the previous method and the start of the JavaDoc.

   }
   /**
    * No parameter quoter function implementation.
    * This will be called when trying to quote an unknown literal.
    * @return    The default unknown character ?.
    */
   public static character quoter()

I would format this like:

   }

   /**
    * No parameter quoter function implementation. This will be called
    * when trying to quote an unknown literal.
    *
    * @return    The default unknown character ?.
    */
   public static character quoter()

14. Please align the asterisks in JavaDoc and make sure there are always asterisks on blank lines:

    * <p>
     * This puts the character value of the variable between
    * double quotes if the value is not unknown or replace it with '?' if the

should be this:

    * <p>
    * This puts the character value of the variable between
    * double quotes if the value is not unknown or replace it with '?' if the

Also note that there was a TAB character in this example which should never be there.

And this:

    *           given in 3 parameter call.

    * @return   The resulting quoted character value of the function.

should be this:

    *           given in 3 parameter call.
    *
    * @return   The resulting quoted character value of the function.

15. When you must reference a casted type multiple times, implementing the cast as a separate line makes the result more readable.

      else if (customQuote instanceof character)
      {
         quoter  = ((character) customQuote).isUnknown()
                   ? null
                   : ((character) customQuote).value;
      }

Is better done as this:

      else if (customQuote instanceof character)
      {
         character custom = (character) customQuote;
         quoter  = custom.isUnknown() ? null : custom.value;
      }

By the way, the character class already has this last code implemented as a helper. So the code rally could have been written like this:

      else if (customQuote instanceof character)
      {
         quoter = ((character) customQuote).tojavaType();
      }

In this case, implementing the cast inline is OK since there is only 1 reference to the cast value.

16. The first line of JavaDoc should be on its own line, not on the same line as the opening comment.

   /**Quoter function implementation for 3 parameters.
    * <p>

should be:

   /**
    * Quoter function implementation for 3 parameters.
    * <p>

17. Generally, we try to avoid using Object parameters.

   public static character quoter(BaseDataType var, Object customQuote)

   public static character quoter(BaseDataType var,
                                  Object mycharQuote,
                                  Object nullVal)
   {

The problem here is that we lose the value of Java's static typing when we implement this way. Future users may make the mistake of passing something other than String or character. The result may surprise them if they didn't read the javadoc carefully. Instead, we tend to implement the full set of properly typed combinations that might be needed.

   public static character quoter(BaseDataType var, String customQuote)
   public static character quoter(BaseDataType var, character customQuote)

   public static character quoter(BaseDataType var,
                                  String customQuote,
                                  String nullVal)
   public static character quoter(BaseDataType var,
                                  String customQuote,
                                  character nullVal)
   public static character quoter(BaseDataType var,
                                  character customQuote,
                                  String nullVal)
   public static character quoter(BaseDataType var,
                                  character customQuote,
                                  character nullVal)

Note also how we reuse the same parameter names in all overloaded versions, so that it is easier for the reader.

I would "reuse" the other variants so that the unwrapping of the 2nd character parameter can be done in only one place. For example:

   public static character quoter(BaseDataType var, character customQuote)
   {
      return quoter(var, customQuote, null);
   }

18. Please don't use names like "mycharQuote". Your use of "customQuote" is good.

#26 Updated by Costin Savin over 11 years ago

Added revised update but for
14:
Couldn't find the wrong alignment

    * <p>
     * This puts the character value of the variable between
    * double quotes if the value is not unknown or replace it with '?' if the


all text matching this description looked aligned
and I also checked

    *           given in 3 parameter call.

    * @return   The resulting quoted character value of the function.

with show whitespaces on and I couldn't see any tab character, maybe it is because of the editor.

- As a solution I deleted those line and rewrote them to be sure.

#27 Updated by Costin Savin over 11 years ago

  • Status changed from WIP to Review

#28 Updated by Greg Shah over 11 years ago

Feedback:

1. The file rules/convert/builtin_functions.rules~ should not be in the update zip.

2. There are still places in character.java where there is a missing blank line between the end of a method and the beginning of the next method's javadoc.

3. This one is wrong:

   public static character quoter(BaseDataType var, character customQuote)
   {
      return quoterWorker(var, customQuote.value, null);
   }

it should be this:

   public static character quoter(BaseDataType var, character customQuote)
   {
      return quoter(var, customQuote, null);
   }

The version you have is invalid if customQuote is unknown. The version I suggest reuses the character unwrapping (no duplication) without losing any functionality.

The same problem exists in:

   public static character quoter(BaseDataType var,
                                  character customQuote,
                                  character customNull)
   public static character quoter(BaseDataType var,
                                  String customQuote,
                                  character customNull)
   public static character quoter(BaseDataType var,
                                  character customQuote,
                                  String customNull)

4. There should always be a space between text and some parenthesized comment:

* doubled(another double quote will be added next to it - Progress

should be:

* doubled (another double quote will be added next to it - Progress

5. In quoterWorker(), why are you splitting the ternary operators onto multiple lines? That is not advised unless the line is going to be longer than 78 chars.

      String nullVal = (customNull != null) 
                       ? customNull 
                       : "?";

should be:

      String nullVal = (customNull != null) ? customNull : "?";

AND this:

      String quoteVal = (customQuote != null) 
                        ? customQuote.substring(0, 1) 
                        : "\"";

Although the total line would be over 78 chars, it can be split more cleanly, to minimize the number of lines:

      String quoteVal = (customQuote != null) ? customQuote.substring(0, 1) 
                                              : "\"";

#29 Updated by Costin Savin over 11 years ago

1. This issue seems to appear when I recheck the files from the zip before submitting, I'm aware of it now.

3.

   public static character quoter(BaseDataType var, character customQuote)
   {
      return quoter(var, customQuote, null);
   }

This is ambiguous and java will not know which method to call unless we use a typed null variable.

To handle unknown character we can have something like:

   public static character quoter(BaseDataType var, character customQuote)
   {
      return quoterWorker(var, customQuote.isUnknown() 
                               ? null 
                               : customQuote.value, null );
   }

is this ok?

#30 Updated by Greg Shah over 11 years ago

   public static character quoter(BaseDataType var, character customQuote)
   {
      return quoter(var, customQuote, null);
   }

This is ambiguous and java will not know which method to call unless we use a typed null variable.

Yes. Just solve it like this:

   public static character quoter(BaseDataType var, character customQuote)
   {
      return quoter(var, customQuote, (String) null);
   }
To handle unknown character we can have something like:

   public static character quoter(BaseDataType var, character customQuote)
   {
      return quoterWorker(var, customQuote.isUnknown() 
                               ? null 
                               : customQuote.value, null );
   }

is this ok?

No. The point of calling the other overloaded quoter() method is to REUSE the custom quote unwrapping code. By doing it this way, you must duplicate that code in multiple places. That is not good. Please just cast the null to the right type to solve the compiler problem.

#31 Updated by Costin Savin over 11 years ago

Attached proposed update

#32 Updated by Greg Shah over 11 years ago

This javadoc:

    * Aditionally a parameter representing a custom unknown
    *  or quoter character replacement is provided.
    *  
    * @param    var 
    *           The BaseDataType variable to operate upon
    * @param    customQuote
    *           The String variable
    *           that represents the custom quoter.Custom unknown 
    *           can only be given in 3 parameter call.
    *
    * @return   The resulting quoted character value of the function.
*/
public static character quoter(BaseDataType var, String customQuote)

should be this:

    * <p>
    * Additionally, a parameter representing a custom quote character
    * is provided.  If not null, then the first character
    * of the given text will be used instead of the " character.
    * <p>
    * A custom unknown value can only be given in the 3
    * parameter call.
    *  
    * @param    var 
    *           The variable to operate upon.
    * @param    customQuote
    *           A variable that contains the custom quote character.
    *
    * @return   The resulting quoted text.
*/
public static character quoter(BaseDataType var, String customQuote)

Note that I have been more careful with my description and with my formatting. I have added more information about the limitation in how the custom quote is used (only the first character is valid). I also removed the embedded type information in the param doc since this is automatically provided by the javadoc processor using the code itself. In other words, you don't need to put that in yourself. By removing it, you can reuse the same javadoc for other variants of the method. If you were to leave it there and then change the type, you might forget to change the javadoc too. Since the javadoc processor will handle that for you, it is best to leave it out.

Please make these same changes for the other 2 parameter method:

public static character quoter(BaseDataType var, character customQuote)

This javadoc:

    * Aditionally two parameters representing custom replacement values for
    * unknown and quoter character are provided.
    * 
    * @param    var
    *           The BaseDataType variable to operate upon.
    * @param    customQuote
    *           The custom character quoter variable.
    * @param    customNull
    *           The custom character unknown 
    *           replacement to be used.
    *
    * @return   The resulting quoted character value 
    *           of the function.
*/ public static character quoter(BaseDataType var,
character customQuote,
character customNull) {

should look like this:

    * <p>
    * Additionally, a parameter representing a custom quote character
    * is provided.  If not null, then the first character
    * of the given text will be used instead of the " character.
    * <p>
    * A custom unknown value can also be provided.  If
    * not null, then the given text will be used instead
    * of the default ?<code>unknown</code> replacement to be used.
    *
    * @return   The resulting quoted text.
    */
   public static character quoter(BaseDataType var,
                                  character customQuote,
                                  character customNull)
   {
</code character.
    * 
    * @param    var
    *           The variable to operate upon.
    * @param    customQuote
    *           A variable that contains the custom quote character.
    * @param    customNull
    *           The custom 

Please use this version with all of the 3 parameter variants.

The quoterWorker() javadoc:

   /**
    * General quoter function implementation. This puts the
    * <code>BaseDataType</code> value between double quotes if the value is
    * not unknown, or replace it with '?' if the value is unknown. If the
    * character variable contains double quotes inside, it will be
    * doubled (another double quote will be added next to it - Progress
    * behaviour). Custom values for unknown or quote character replacements
    * can be given.
    * 
    * @param    var
    *           The <code>BaseDataType</code> variable to operate upon.
    * @param    customQuoter.
    *           The custom quoter character to be used.If <code>null</code> default quoter
    *           value is used.
    * @param    customNull
    *           The custom unknown text replacement to be used. If <code>null</code>
    *           default value is used.

should look like this:

   /**
    * This is the core quoter function implementation.  This puts the text value
    * of the variable between double quotes if the value is not unknown, or it
    * will replace it with the <code>?</code> character if the value is
    * <code>unknown</code>. If the variable is a <code>character</code> type,
    * any contained double quote character will be doubled (another double quote
    * character will be added next to it - Progress behaviour). 
    * <p>
    * The caller can provide replacement characters to use instead of the double
    * quote character (&quot;) and instead of the default <code>unknown</code>
    * character <code>?</code>.
    * 
    * @param    var
    *           The variable to operate upon.
    * @param    customQuoter
    *           The custom quote character to be used. If <code>null</code> the
    *           default &quot; character is used.
    * @param    customNull
    *           The custom <code>unknown</code> text replacement to be used. If
    *           <code>null</code>, the default <code>?</code> is used.

Otherwise the code looks good.

I don't see a need to do runtime regression testing. Please confirm that there are no differences in the converted Majic code. If so, then once you post the javadoc changes and they pass review, then it will be done.

#33 Updated by Costin Savin over 11 years ago

Added revised proposed update. From what I understand there are plans to move code that is not character specific from the class(task #1884 i think).
This also fits the profile. should the logic be moved to another class?( maybe the class of the Quoter utility ).

#34 Updated by Greg Shah over 11 years ago

Feedback:

This javadoc for the 2 parameter version needs to be changed:

    * Aditionally a parameter representing a custom unknown
    * or quoter character replacement is provided. If not null, then the first
    * character of the given text will be used instead of the " character.

to this:

    * Additionally a parameter representing a custom quote character 
    * replacement is provided.  If not null, then the first
    * character of the given text will be used instead of the "
    * character.

Otherwise it is fine. Make this last change and lets get it tested.

#35 Updated by Costin Savin over 11 years ago

Added proposed update, started the regression test

#36 Updated by Costin Savin over 11 years ago

Attached wrong update before. This is the correct one

#37 Updated by Costin Savin over 11 years ago

Done conversion regression testing, there are no differences.

#38 Updated by Greg Shah over 11 years ago

There is still a javadoc problem.

    * Aditionally a parameter representing a custom  quote character 
    * replacement is provided. If not null, then the first
    * character of the given text will be used instead of the " 
    * character.

The word Aditionally should be Additionally.

null should be

" should be written as an HTML entity (using ampersand + quot;)

My previous posts also specified the entity change, but the HTML processing of the Redmine notes just made it look like a double quote character.

#39 Updated by Costin Savin over 11 years ago

Added update. I assumed null -> <code>null</code>

#40 Updated by Greg Shah over 11 years ago

OK, it looks good.

Good ahead and do the following:

1. Work with Constantin to get the update applied to staging.
2. Check it into Bazaar.
3. Distribute the update via email to the team.

#41 Updated by Constantin Asofiei over 11 years ago

Applied to staging and P2J was rebuilt. MAJIC was not reconverted or rebuilt.

#42 Updated by Greg Shah over 11 years ago

  • % Done changed from 0 to 100
  • Status changed from Review to Closed

#43 Updated by Greg Shah over 11 years ago

I forgot to ask you to update the documentation. Please update the builtin functions table in the expressions.odt chapter of the conversion reference.

#44 Updated by Costin Savin over 11 years ago

Added the entry to expressions.odt

#45 Updated by Costin Savin over 11 years ago

A quoter version which supports String instead of BaseDataType should probably be added to be reused for places where we don't have a BaseDataType (In my case I was trying to reuse the logic in OS-Dir quoting of the filename and path).
I also realize now I overlooked the simplest case, since with it's purpose quoter was unlikely to receive anything that doesn't have a base data type, but: quoter("Just a String") is valid and it will convert to character.quoter(String s) which is not implemented.
Should I add the String overloads?

#46 Updated by Greg Shah over 11 years ago

Yes.

#47 Updated by Costin Savin over 11 years ago

Added proposed update with the String overloaded methods.

#48 Updated by Greg Shah over 11 years ago

Feedback:

1. Remove the builtin_functions.rules from your zip. There are no changes there.

2. The character.java is missing a history entry. You can't reuse the old entry because the code was already tested and checked in.

3. This javadoc on line 3937 in the quoter(String) method is wrong:

    * This puts the character value of the variable between
    * double quotes if the value is not unknown  or replace it 
    * with '?' if the value is unknown.

There is no character value since this is a String. And a String can never be unknown. This should just explain what will happen to the string when quoter operates on it.

The same problem exists on:
- line 3993 in the quoter(String, String) method
- line 4049 in the quoter(String, character) method
- line 4115 in the quoter(String, character, character) method
- line 4187 in the quoter(String, String, character) method
- line 4256 in the quoter(String, character, String) method
- line 4325 in the quoter(String, String, String) method

Otherwise, these changes look fine.

#49 Updated by Costin Savin over 11 years ago

Added proposed update

#50 Updated by Greg Shah over 11 years ago

OK, it looks good. These additions should not be able to cause any runtime regressions. So I am going to bypass regression testing for this change.

Do the following:

1. Work with Constantin to get the update applied to staging.
2. Check it into Bazaar.
3. Distribute the update via email to the team.

#51 Updated by Constantin Asofiei over 11 years ago

I've applied the update to staging and P2J is rebuilt.

#52 Updated by Greg Shah over 7 years ago

  • Target version changed from Milestone 7 to Runtime Support for Server Features

Also available in: Atom PDF