sis-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "M. Le Bihan (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (SIS-180) Place a crude JDBC driver over Dbase files
Date Tue, 14 Oct 2014 19:40:34 GMT

    [ https://issues.apache.org/jira/browse/SIS-180?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14171402#comment-14171402
] 

M. Le Bihan commented on SIS-180:
---------------------------------

* Please configure your IDE for using spaces rather than tabulations, since 
not all editors have the same tabulation width.
Of course !

* Remove the {{public}} modifier for every classes that do not need to be 
accessed from outside the package.
Ok.

* Replace star imports by explicit imports, unless there is really a lot of 
them (e.g. more than 8 from the same package).
Ok. But I don't like this rules, even if it is common. Stars allow to see 
less lines of imports, and avoid folding these imports. When the imports are 
fold, people don't check their dependencies and don't detect that their 
classes begin to have some links with some unexpected packages.
Most of the time, I am wiping imports from other's code because they are 
unused, but they don't know this, because they kept them fold.

* Minor grammatical note: Javadoc convention asks us to use the third person 
in the first sentence of method javadoc (e.g. "Format*s*" instead than 
"Format").
Ok.

* Some classes have {{\@SuppressWarnings("unused")}} annotation in front of 
every arguments, which make method signature very verbose. {{"unused"}} is 
not a value recognized by the Oracle {{javac}} compiler (as of JDK8). I 
presume that this is an Eclipse-specific one? If this annotation is really 
desired, I suggest to put it at the class level at least in {{AbstractFoo}} 
classes, for avoiding to repeat it hundred of times.
I didn't know that only eclipse where using the unused SuppressWarnings. I 
always set quite all the warnings detection on and take time to resolve all 
the troubles that Eclipse detects. Among them, unused variables are to be 
checked, because they often show dead code or unexpected actions. So when I 
know that a parameter is not used yet (it's not often, its only specific to 
the Unimplemented classes that are really special) I put this 
@SuppressWarning on the parameters.

* {{unsupportedOperation}} method excepts the source class and method in 
argument. But this information is not complete at least in 
{{AbstractResultSet}}. We could complete them... but given that this 
information duplicates the stack trace information, maybe it could also be 
completely omitted.

* It may be worth to take a look at the Javadoc of all JDBC methods and see 
if some of them are optional. I suspect that not all methods need to throws 
a {{SQLFeatureNotSupportedException}}.
They do not need it. But I want the caller to be immediately stopped by this 
exception in order to know which method needs to be implemented quickly. If 
I keep void methods (with only logs or nothing), it will become very 
difficult to follow what the JDBC classes are willing to do, what methods 
are called and when, especially at the time we will create a Datasource and 
attempt JPA handle it.
At this time where implementation of the JDBC driver is at its beginning, 
its really easy for me to loose myself. I like having stops put everywhere, 
in order to discover that I felt in a case I didn't expected.

* In the Javadoc, I guess that all the {{\@see the_implemented_JDBC_method}} 
were inserted by Eclipse? Since the Javadoc tool generates this information 
automatically in the "Specified by" section, those automatically generated 
{{\@see}} tags are redundant and could be omitted.
Ok.

I will be able to do some changes more easily at the time I can commit on 
the project.
I've send my icla to the Apache secretary.


Regards,

Marc Le Bihan

-----Message d'origine----- 
From: Martin Desruisseaux (JIRA)
Sent: Tuesday, October 14, 2014 6:52 PM
To: mlebihan29@gmail.com
Subject: [jira] [Commented] (SIS-180) Place a crude JDBC driver over Dbase 
files


    [ 
https://issues.apache.org/jira/browse/SIS-180?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14171208#comment-14171208

 ]

Martin Desruisseaux commented on SIS-180:
-----------------------------------------

Committed on the JDK8 branch. But before to continue on JDBC support, would 
it be possible to do some consolidation work?

I applied the following changes:

* Renamed package as {{org.apache.sis.internal.shapefile.jdbc}}, so those 
classes will be hidden from public API.
* Renamed {{XXXUnimplementedFeature}} classes as {{AbstractXXX}}.
* Omitted {{DBaseJDBCDriverUnhandledOperationException}}. Used 
{{java.sql.SQLFeatureNotSupportedException}} instead.
* Removed all {{throw new RuntimeException("Should not have been there")}} 
lines since they were unnecessary (note: {{throw new AssertionError(...)}} 
would have been more accurate). See the code for the simplification trick.
* Defined {{InvalidDbaseFileFormatException}} as a subtype of 
{{SQLNonTransientException}} instead than {{SQLException}}.

Would you have a chance to apply the following proposed consolidation, if 
you agree?

* Please configure your IDE for using spaces rather than tabulations, since 
not all editors have the same tabulation width.
* Remove the {{public}} modifier for every classes that do not need to be 
accessed from outside the package.
* Replace star imports by explicit imports, unless there is really a lot of 
them (e.g. more than 8 from the same package).
* Minor grammatical note: Javadoc convention asks us to use the third person 
in the first sentence of method javadoc (e.g. "Format*s*" instead than 
"Format").
* Some classes have {{\@SuppressWarnings("unused")}} annotation in front of 
every arguments, which make method signature very verbose. {{"unused"}} is 
not a value recognized by the Oracle {{javac}} compiler (as of JDK8). I 
presume that this is an Eclipse-specific one? If this annotation is really 
desired, I suggest to put it at the class level at least in {{AbstractFoo}} 
classes, for avoiding to repeat it hundred of times.
* {{unsupportedOperation}} method excepts the source class and method in 
argument. But this information is not complete at least in 
{{AbstractResultSet}}. We could complete them... but given that this 
information duplicates the stack trace information, maybe it could also be 
completely omitted.
* It may be worth to take a look at the Javadoc of all JDBC methods and see 
if some of them are optional. I suspect that not all methods need to throws 
a {{SQLFeatureNotSupportedException}}.
* In the Javadoc, I guess that all the {{\@see the_implemented_JDBC_method}} 
were inserted by Eclipse? Since the Javadoc tool generates this information 
automatically in the "Specified by" section, those automatically generated 
{{\@see}} tags are redundant and could be omitted.





--
This message was sent by Atlassian JIRA
(v6.3.4#6332) 



> Place a crude JDBC driver over Dbase files
> ------------------------------------------
>
>                 Key: SIS-180
>                 URL: https://issues.apache.org/jira/browse/SIS-180
>             Project: Spatial Information Systems
>          Issue Type: Improvement
>          Components: Storage
>    Affects Versions: 0.5
>            Reporter: M. Le Bihan
>            Assignee: Martin Desruisseaux
>            Priority: Minor
>             Fix For: 0.5
>
>         Attachments: src.zip
>
>
> It would be useful to be able to query DBF content through SQL.
> But there are no free drivers available for the old _Dbase 3_ format.
> The first step is to create short implementations of _Connection_, _Statement_, _ResultSet_,
_ResultSetMetadata_ interfaces for a JDBC using our _Database_ class as core binary loader
at the begining.
> The main difficulty is to respond to a SQL request, and first : being able to analyze
it to understand what is expected.
> The SQL request analysis is a very strong job, but I suggest to ease it a lot by relying
on _AntLR_ API for grammar analysis, associated with a BNF grammar file, maybe taken from
^1^ or from elsewhere (grammars are of public domain).
> The goal of this current JIRA is only to be able to perform a 
> _SELECT * FROM <shapefile layer name>_
> The WHERE clause or the selection of fields, will come later in other JIRA.
> No transactions, classic _Statement_ only.
> _PreparedStatement_ would be also implemented later (another JIRA).
> Of course, this improvment can be discarded if an open source or free driver is discovered,
that would allow us to execute SQL requests on DBase 3 easily.
> ^1^ For example, [http://www.savage.net.au/SQL/] has some BNF, but maybe elsewhere they
will more compliant with AntLR.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Mime
View raw message