Jody Garnett ha scritto: ... >> Implementation wise, this triggers the first issue: >> that method does not tell what is wrong, but only >> that something is wrong. >> A different approach could be to have a >> validate() throws IllegalAttributeException >> and have the exception report exactly what's wrong >> with the feature. >> > Difficulty is that this either needs to be a structures exception > message (indicating say the xpath to the problem and kind of problem) or > we should > provides a utility method to do this kind of check.
I was leaning towards a structured exception message. Like, I'd prefer to keep all the validation logic in one place, and possibly in an obvious one, splitting it around does not look nice to me. IllegalAttributeException is already structured, it has the descriptor of the failing attribute and the value that does not conform to it, too bad it's not exposing them, but it's just a matter of adding a couple of getters. >> Moreover, even having that method, someone might want to have >> continuous checks on the feature model >> like we do now, that is, have the builders and the model throw >> exceptions when an invalid >> attribute value is provided. >> >> Do we really want to do that? Or we just tell the world that >> validation checks must be explicit, that is, never ask the model to do >> them automatically? >> > I had pictured a useful compromise; > - have validation off by default > - have always-on-validation as a "strict" option to help debug problems > > Given the choice; validation off is the way to go. If the strict option is meant to help debug only, what about a system variable to enable it? >> If on the contrary we believe it's important and we allow the user to >> customize that behaviour, Jody was suggesting to do so by providing >> two different feature factories, >> one that builds non validating features, the other that builds >> validating ones... which in the end would boil down to >> parametrize the SimpleFeatureImpl with a flag that enables >> validation when attributes are set. >> It would also require the builder static utility methods, which >> are very commonly used in the datastores, to accept a factory. >> To make it effective it would mean we'd need to add a new >> Query hint (the feature factory, which is not there) and use it >> in all data stores. >> Yet that would not be all, those methods are used in >> many many places (SimpleFeatureBuilder.build(...) is used in 179 >> different places according to Eclipse), do we want add a factory >> param in each place? Just have a look in DataUtilities class >> to get an idea. >> > It was these bigger picture questions that were being talked about as > the builder was introduced. The SimpleFeatureBuilder.build method is > always a short cut hack; mostly to enable the DataUtility methods to > work ... > How about for these cases we consider "validation off" to be the rule > rather than the exception. >> Given that most non datastore methods are transformation related ones, >> we could use the same factory that built the original features, adding >> a getFactory() method to the Property interface... >> thought of course that would be another geoapi change, would >> require even more changes around, well, you get the picture. >> >> It seems to me this kind of change would require at least a one >> day sprint... what do we do in the meantime? >> > > Backing up: > - Our goal was to make the FeatureTypes static utility method available > for people to find; we have now done that correct? More or less, we do if we provide a validate() method that throws an exception giving people the full meal, otherwise we have to keep that Types utility class around, have isValid() call it, and then have the user call it again if he wants to know what is not valid, bleck... > - For detailed reporting on why something is invalid - a good idea - we > should treat that as a separate problem; it is something people can use > a utility method for. Once we understand the problem we can consider > adding it to the API. Code first then API. I don't see it as a separate problem, I think having a separate API just to grab the details of what was wrong is not a good idea. > - We should only worry about the use of SimpleFeatureBuilder as a > builder ie the static methods on SimpleFeatureBuilder and in > DataUtilities are assumptions used throughout our library; they saved us > time when making the migration. You should be able to inline > SimpleFeatureBuilder.build() as needed... > - For the static methods we should make use of a system wide > FetaureFactory - assuming validation off by default. System wide == configurable? Where is that located? Or is it just a hard coded default in the builder? > Aside: The idea of remembering the factory seems like a little bit of > gain; while I had similar thoughts they come from a different problem > When working with Shapefiles only a limited set of content is supported > (max string length etc...). Having > ShapefileDataStore.getFeatureTypeFactory() return a factory that knows > about these limitations is a sensible way to go... Uuh, not sure... the ideas of factories is to put the user in control not the lower levels? Cheers Andrea ------------------------------------------------------------------------- Sponsored by: SourceForge.net Community Choice Awards: VOTE NOW! Studies have shown that voting for your favorite open source project, along with a healthy diet, reduces your potential for chronic lameness and boredom. Vote Now at http://www.sourceforge.net/community/cca08 _______________________________________________ Geotools-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/geotools-devel
