Re: [gnome-db] Libgda Firebird provider patch

[Rodrigo Moya <rodrigo gnome-db org>]

On Mon, 2004-07-05 at 15:11 +0000, Jeronimo Albi wrote:
>   This is my first Firebird patch (070504-0) for libgda. Right now i'm 
> working to add
> support for Blob type and to some other schemas. I have a test 
> application runnig,
> i should be sending it to the list during this week.
>
ok, thanks a lot for the patch. Some comments below.

> -#define PARENT_TYPE GDA_TYPE_SERVER_PROVIDER
> +#ifdef PARENT_TYPE
> +    #undef PARENT_TYPE
> +#endif
>
why is this needed? Where is PARENT_TYPE defined before this?

> +		{ "double precision", "", "Double precision number", GDA_VALUE_TYPE_DOUBLE }
> 
is "double precision" the string you use in SQL commands? This string is used in
some places (Mergeant IIRC) to create SQL commands for creating tables, so the string
needs to be exactly what you would use in those SQL commands.

> +static GdaDataModel *
> +fb_get_tables (GdaConnection *cnc,
> +		GdaParameterList *params,
> +		gboolean show_views)
> +{
> +	GList *reclist, *value_list;
> +	GdaDataModel *recset = NULL;
> +	GdaTransaction *transaction;
> +	GdaCommand *command;
> +	GdaValue *value;
> +	GdaRow *row;
> +	GdaParameter *par = NULL;
> +	gchar *sql, *sql_cond, *table_name;
> +	gint i;
> +	gboolean systables = FALSE;
> +
> +	g_return_val_if_fail (GDA_IS_CONNECTION (cnc), NULL);
> +        
> +	transaction = gda_transaction_new ("temp_transaction");
> +	if (gda_connection_begin_transaction (cnc, transaction)) {
> 
why do we need a transaction for reading the tables?

> +		*dpb++ = isc_dpb_user_name;
> +		*dpb++ = strlen(fb_user);
> +		strcpy(dpb, fb_user);
> +		dpb += strlen(fb_user);
> 
please use our coding standards, which say to leave a space between the function
name and the opening '('.

>  gda_firebird_provider_drop_database (GdaServerProvider *provider,
> -				      GdaConnection *cnc,
> -				      const gchar *name)
> +				     GdaConnection *cnc,
> +				     const gchar *name)
>  {
> +	GdaFirebirdConnection *fcnc;
> +    
> +	g_return_val_if_fail (GDA_IS_FIREBIRD_PROVIDER (provider), FALSE);
> +	g_return_val_if_fail (GDA_IS_CONNECTION (cnc), FALSE);
> +
> +	fcnc = g_object_get_data (G_OBJECT (cnc), CONNECTION_DATA);
> +	if (!fcnc) {
> +		gda_connection_add_error_string (cnc, _("Invalid Firebird handle"));
> +		return FALSE;
> +	}
> +    
> +	/* Connection must be open for database drop */	
> +	if (gda_connection_is_open (cnc)) {
> +  		if (isc_drop_database (fcnc->status, &(fcnc->handle))) {
> 
this is wrong, you should not drop the current database, but the database specified
by the 'name' argument. So, usually, you'll just use a DROP DATABASE SQL command.

> +	//Get statement info
> 
please use /* .. */ for comments.

> +	/* Free cursors */
> +	isc_dsql_free_statement (fcnc->status, &(recset->priv->sql_handle), DSQL_drop);
> +	/* Free SQL result */
> +	fb_sql_result_free (recset->priv->sql_result);
> +	return FALSE;
> 
also, to make code more legible, we usually leave a blank line before comments.

> +static GdaFieldAttributes *
> +gda_firebird_recordset_describe_column (GdaDataModel *model, 
> +					gint col)
> +{
> +	return NULL;
> +}
> +
this function is mandatory, so you should really implement it.

the rest seems to look ok, apart from many formatting issues, so please correct
those and send a new patch, please.

cheers