mdbtools-dev

[mdb-dev] mdb-sql: Readline Completion Support

[Kyle R. B. <mo...@vo...>]

I've been using mdb-sql and since you've already got basic readline
support, I went and added in completion support to the shell.

I looked at what mdb-tables was doing to list the tables in the database
catalog and implemented similar functionality so table names are
included in the completion list.

The way the completion is implemented, it will only return candidates if
you've typed at least the first letter (or more) of one of the words.  If
the sql parsing strategy were changed so that partial parses could be 
recognized, then we could start looking at state-based completion - 
the idea being if you type: 'select ' and then hit tab, only table
names should be considered (and at that point a double tab could
just return the list of all of the tables).

A patch should be attached.


Best regards,
Kyle



-- 

------------------------------------------------------------------------------
Wisdom and Compassion are inseparable.
        -- Christmas Humphreys
mo...@vo...                            http://www.voicenet.com/~mortis
------------------------------------------------------------------------------

Re: [mdb-dev] mdb-sql: Readline Completion Support

[Michael W. <mw...@it...>]

Hi

Since I am basically just a lurker on this list, feel free to ignore
these comments:

1.)  It's generally easier to review patches if each patch does one
thing.  i.e. you should keep your case insensitive SQL patch separate
from your completion patch and attach both to your message if necessary.

2.)  Gratuitous whitespace changes get in the way of what your patch is
doing.  The reviewer has to waste time looking through the whitespace
changes to see if you made any other changes in that section besides
just the whitespace changes.

[snip]
>  int yywrap()
>  {
> -	return 1;
> +  return 1;

e.g. here.  There's no functional change.  This is just noise.

>  void yyerror(char *s)
>  {
> -	fprintf(stderr,"Error at Line : %s near %s\n", s, yytext);
> +  fprintf(stderr,"Error at Line : %s near %s\n", s, yytext);

And here too.

>  #if 0
>  int main(int argc, char **argv)
>  {
>  int i;
>  
> -	g_sql = mdb_sql_init();
> -	yyin = stdin;
> -	if (yyparse()) {
> -		fprintf(stderr, "Couldn't parse SQL\n");
> -		exit(1);
> -	}
> -	mdb_sql_dump(g_sql);
> -	mdb_sql_exit(g_sql);	
> +  g_sql = mdb_sql_init();
> +  yyin = stdin;
> +  if (yyparse()) {
> +    fprintf(stderr, "Couldn't parse SQL\n");
> +    exit(1);
> +  }
> +  mdb_sql_dump(g_sql);
> +  mdb_sql_exit(g_sql);  
[snip]

And the same here.

-- 
Michael Wood <mw...@it...>

Re: [mdb-dev] mdb-sql: Readline Completion Support

[Kyle R. B. <mo...@vo...>]

> Since I am basically just a lurker on this list, feel free to ignore
> these comments:
> 
> 1.)  It's generally easier to review patches if each patch does one
> thing.  i.e. you should keep your case insensitive SQL patch separate
> from your completion patch and attach both to your message if necessary.

That's good advice, thanks.  I had done several things in my local working 
copy of the software and didn't pay enough attention when I created the 
patch.  If the project wants the readline completion code, I can re-submit 
a patch based solely on that feature.

> 2.)  Gratuitous whitespace changes get in the way of what your patch is
> doing.  The reviewer has to waste time looking through the whitespace
> changes to see if you made any other changes in that section besides
> just the whitespace changes.

Another good point.  I had inadvertantly modified some lines while trying
to learn the code.  As you have pointed out, they shouldn't be included in
patches.  If either of these features are desired by the developers for
inclusion I can break them into their own patches.


Thank you for the advice.

Kyle R. Burton


-- 

------------------------------------------------------------------------------
Wisdom and Compassion are inseparable.
        -- Christmas Humphreys
mo...@vo...                            http://www.voicenet.com/~mortis
------------------------------------------------------------------------------

Re: [mdb-dev] mdb-sql: Readline Completion Support

[Mark K. <kil...@nb...>]

Michael Wood wrote:
> Hi
> 
> Since I am basically just a lurker on this list, feel free to ignore
> these comments:

Me too.  I want to use the MDB tools to look at a db under my Linux box 
(I *hate* rebooting into Windows!)

> 2.)  Gratuitous whitespace changes get in the way of what your patch is
> doing.  The reviewer has to waste time looking through the whitespace
> changes to see if you made any other changes in that section besides
> just the whitespace changes.

In most diff programs, you can ignore whitespace changes.  Take the GNU 
diff on my system:

-b     Ignore changes in amount of white space.
-B     Ignore changes that just insert or delete blank lines.

White space changes are actually a good thing, if they improve 
readability, so I wouldn't poo-poo them entirely...

You can add these options to your .cvsrc if you are using cvs.

Just a hint...

MK
-- 
"An expert is a man who has made all the mistakes which can be made,
in a narrow field."   - Niels Bohr
- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
Mark Kilfoil                                   <kil...@nb...>
  Software Designer/Developer, Radio Host, Gamer, Curmudgeon, Oddball
     From the FINE mega-city of Fredericton, New Brunswick, Canada
- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -