> The class has a setter, which would have to be removed (instead of just being deprecated). > That's not our normal API removal cycle (first deprecate, then remove), but it is allowed in exceptional cases. > In this case I don't see a valid enough reason to break downstream apps that might be using the setter, so -1 > on the straight removal of the method. > As noted in https://github.com/geotools/geotools/pull/3516 I'm skeptical about the deprecation too, but not > strongly against it, if some other core dev takes responsibility for it I'm not going to oppose.
Not sure if we discuss about the same class, its about SLDParser and this request might be confusing due to the fact of existing pull request for ExpressionDOMParser. However, I guess its might be easier to talk about the code and haveing a look at my proposal. Please see my commit https://github.com/fgdrf/geotools/commit/9de2c9c809818de17eb29ca8bbd8c229ffaf4eec which has not test-cases yet but shows the mentioned code in context and in relation to the exiting code on main-branch. if its easier to move discussion I could also make a draft pull request out of it. Hope this helps -- Frank Am Mo., 24. Mai 2021 um 10:12 Uhr schrieb Andrea Aime < andrea.a...@geo-solutions.it>: > On Mon, May 17, 2021 at 8:48 AM Frank Gasdorf via GeoTools-Devel < > geotools-devel@lists.sourceforge.net> wrote: > >> Hi folks, >> >> I stumbled about the SLDParser while I wass looking for final field >> initializations with CommonFactoryFinder.getXXX >> >> However, I'm not sure about whether it was by intension but internally >> ther is a cast from FilterFactory to FilterFactory2 on initailization an >> ExpressionDOMParser. This looks strange because other implementations (and >> contributions via SPI) might implement FilterFactory but not FilterFactory2. >> > > Hard to say, even going back before the Java 11 compatibility work, the > cast was already there at the beginning of > the git history, 11 years ago. > Did a very quick check in SVN. the earliest I can find CommonFactoryFinder > is in 2.7.x, the cast was already like that: > > https://svn.osgeo.org/geotools/branches/2.7.x/modules/library/api/src/main/java/org/geotools/factory/CommonFactoryFinder.java > > We might not find someone that remembers why things were done this way. > > >> >> In addition a filed expressionDomParser is initialized with static field >> FF but not used in every method the same way. Sometimes an >> ExpressionDOMParseris created while needed with the mentioned cast of >> internal FilterFactory, sometimes it uses >> CommonFactoryFinder.getFilterFactory2() and some methods using the internal >> field expressionDomParser >> >> What do you think about changing the constructor as follows >> >> public SLDParser(StyleFactory factory, FilterFactory filterFactory) { >> this.factory = factory; >> this.ff = filterFactory; >> if (filterFactory instanceof FilterFactory2) { >> this.filterFactory2 = (FilterFactory2)filterFactory; >> } else { >> this.filterFactory2 = CommonFactoryFinder.getFilterFactory2(); >> } >> this.expressionDOMParser = new >> ExpressionDOMParser(filterFactory2); >> this.onlineResourceLocator = new DefaultResourceLocator(); >> } >> >> and change the member expressionDOMParser to a final field. All other >> places where ExpressionDOMParsers ar created should be changed to use final >> field expressionDOMParser instead. >> > > The class has a setter, which would have to be removed (instead of just > being deprecated). > That's not our normal API removal cycle (first deprecate, then remove), > but it is allowed in exceptional cases. > In this case I don't see a valid enough reason to break downstream apps > that might be using the setter, so -1 > on the straight removal of the method. > As noted in https://github.com/geotools/geotools/pull/3516 I'm skeptical > about the deprecation too, but not > strongly against it, if some other core dev takes responsibility for it > I'm not going to oppose. > > Regards, > > Andrea Aime > > == GeoServer Professional Services from the experts! Visit > http://goo.gl/it488V for more information. == Ing. Andrea Aime @geowolf > Technical Lead GeoSolutions S.A.S. Via di Montramito 3/A 55054 Massarosa > (LU) phone: +39 0584 962313 fax: +39 0584 1660272 mob: +39 339 8844549 > http://www.geo-solutions.it http://twitter.com/geosolutions_it > ------------------------------------------------------- *Con riferimento > alla normativa sul trattamento dei dati personali (Reg. UE 2016/679 - > Regolamento generale sulla protezione dei dati “GDPR”), si precisa che ogni > circostanza inerente alla presente email (il suo contenuto, gli eventuali > allegati, etc.) è un dato la cui conoscenza è riservata al/i solo/i > destinatario/i indicati dallo scrivente. Se il messaggio Le è giunto per > errore, è tenuta/o a cancellarlo, ogni altra operazione è illecita. Le > sarei comunque grato se potesse darmene notizia. This email is intended > only for the person or entity to which it is addressed and may contain > information that is privileged, confidential or otherwise protected from > disclosure. We remind that - as provided by European Regulation 2016/679 > “GDPR” - copying, dissemination or use of this e-mail or the information > herein by anyone other than the intended recipient is prohibited. If you > have received this email by mistake, please notify us immediately by > telephone or e-mail.* >
_______________________________________________ GeoTools-Devel mailing list GeoTools-Devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/geotools-devel
