Menu

#609 (ok 3.5) Added more documentation links to SQL parser

closed-accepted
1
2012-04-07
2011-03-29
No

I added documentation links to almost every SQL sentence in SQL pretty printer

Discussion

  • Dieter Adriaenssens

    Thanks for the patch, could you recreate the patch without changing the indentation/spacing of the existing code? It is very difficult to see what parts are unchanged and the parts you added. Thanks.

     
  • Michal Čihař

    Michal Čihař - 2011-03-30

    Also comments like "I add here some ..." are pretty useless, that's what version control for, please remove them.

     
  • Mateusz Lewandowski

    I've done what you are asking about. But in the first patch file I not only added some functionality, but also clean up the code which was really unpleasantly written in my opinion. So, in the second patch file you can easier see what I really done, but if you want to apply my patch in next phpMyAdmin release, I suggest to use the first patch.

     
  • Dieter Adriaenssens

    About your patch :

    I got some whitespace warnings when applying your patch, you can track these when doing a git diff. (red full bars in color mode, can't miss them. ;))
    Please use parentheses even if it is for only one statement. It makes the code better readable and avoids future problems.
    It is OK, even encouraged, to write comments explaining why something is done, but please use a passive phrasing : f.e. 'This was done because' in favor of 'I did this because'

    Try to use correct English (grammar and vocabulary) in comments,descriptions, variables and function names, ... (typo's can always happen of course, but try to avoid them (review your own patch before committing) and if you're not a native speaker mistakes can happen, but pay attention) : bad example in this patch : 'this switch wos very ugly and hart to understand'

    There is very much going on in your patch (even in the second one). It is OK to clean up the code, but maybe you can create separate patches/commits for it, one introducing the new functionality, another one (or more than one) cleaning up the code, each with a description (used in commit message) of what is done in that patch. Try not to do too much in one patch. ;)

    Maybe create a separate patch for the 'number_of_tokens' replacement, it makes it easier to follow what is changed.
    Same for replacing of switches and if's if nothing functional has changed, although I'm not convinced of every if and switch you have replaced. It is true a complete if-then-else takes a lot of space, and can be written more compact and is maybe better readable by experienced developers, but usually a full if is more easy to read.

    If you could split it into more patches, I would be happy to review it further. BTW : You can do this by doing several commits in your fork/repository and then exporting them as separate patches using the git format-patch command.

     
  • Mateusz Lewandowski

    I divided my patch as you wished. In the sqlPrettyPrinter.zip archive I have put 4 patches, the first one just reorders some cases in switches to group similar cases; the second patch replaces switches by if's (you wrote that sometimes big swiches are better, I agree with you, but when in a one big switch there are other switches, the code may be hard to read); the third patch adds documentation links; the fourth one is just intendation change, I don't know whether it is a good idea to apply it, but in my opinion, the code with last patch looks better.
    I also make separate patch only for documentation links, which I added without cleaning up the code.
    I also added documentation links for others sql operators, now all functions and operator have links to the documentation.

     
  • Dieter Adriaenssens

    It's easier to see now what you've added.

    But :

    * I don't agree with this replacement :

    - if (($i + 1) < $arraysize) {
    - // array_push($typearr, $arr[$i + 1]['type']);
    - $typearr[4] = $arr[$i + 1]['type'];
    - } else {
    - //array_push($typearr, null);
    - $typearr[4] = '';
    - }
    + $typearr[4]=($i + 1)<$number_of_tokens ? $arr[$i + 1]['type'] : '';

    the comments should be removed of course, but I think the 'if' is better readable than the replacement.

    * try to always use English terms :

    I guess 'nazwa' is Polish for 'name'?

    * I think something went wrong with '$number_of_tokens' replacement, in the if statement where it is set, the 'else' part is obsolete now. (But I guess you missed this when separating the patches)

     
  • Mateusz Lewandowski

    Archive with divided patch

     
  • Mateusz Lewandowski

    You are right about 'nazwa' and '$number_of_tokens' which I missed while separating patches.
    About this replacement:
    - if (($i + 1) < $arraysize) {
    - // array_push($typearr, $arr[$i + 1]['type']);
    - $typearr[4] = $arr[$i + 1]['type'];
    - } else {
    - //array_push($typearr, null);
    - $typearr[4] = '';
    - }
    + $typearr[4]=($i + 1)<$number_of_tokens ? $arr[$i + 1]['type'];

    I use this construction very often, I do like and understand it. I know that most people prefer if's instead of '? : ', so, I return to if in my patch.
    I updated patches in archive.

     
  • Michal Čihař

    Michal Čihař - 2011-04-06

    I fails to see reason for converting case to if and reordering it. It won't make code easier to read, while it will most likely make the code to perform worse.

     
  • Mateusz Lewandowski

    In my opinion, which construction will be easier to read, depends on a person. I know many people who prefer if's instead of very big switch, and a few who prefer switch. About performance, I think that it is not that easy to say which code is faster, it depends on a programming language, and the difference between this constructions shouldn't be big.
    But this is only my opinion, that’s why I uploaded also patch with only necessary changes.

     
  • Michal Čihař

    Michal Čihař - 2011-04-18

    Thanks, it is much nicer to review. Last think I would suggest to improve is moving the definition of the mapping to sqlparser.data.php

     
  • Michal Čihař

    Michal Čihař - 2011-04-18
    • assigned_to: nobody --> nijel
     
  • Michal Čihař

    Michal Čihař - 2011-06-06
    • priority: 5 --> 1
    • summary: Added more documentation links to SQL parser --> (ok 3.5) Added more documentation links to SQL parser
    • status: open --> open-accepted
     
  • Michal Čihař

    Michal Čihař - 2011-06-06

    Most of the patch was merged to repository, though I did have to do some adjustments (like placing of the data to different file).

     
  • Marc Delisle

    Marc Delisle - 2012-04-07
    • status: open-accepted --> closed-accepted