Menu
▾
▴
[Cdk-devel] [ cdk-Patches-3509417 ] Dynamic IO Settings
|
From: SourceForge.net <no...@so...> - 2012-03-22 06:15:29
|
Patches item #3509417, was opened at 2012-03-20 09:10 Message generated for change (Comment added) made by egonw You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=3509417&group_id=20024 Please note that this message will contain a full copy of the comment thread, including the initial issue submission, for this request, not just the latest update. Category: None Group: None Status: Open Resolution: None Priority: 5 Private: No Submitted By: John May (jwmay) Assigned to: Egon Willighagen (egonw) Summary: Dynamic IO Settings Initial Comment: I was using IOSetting's class for the recent patch to IteratingMDLReader when I spotted lots of redundant code in the base reader/writer classes. The settings implementation was also quite constrained requiring access to a fixed size array via 'getIOSettings()'. I thought it would be nice to try and simplify the API by removing redundant code (mainly listener handling) and also added some management of IOSettings. The main bonus is that you can now inherit settings and from super class readers/writer and also add/get specific settings easier (for example force read of 3D coordinates pops up more then once). The existing usage is still valid and so all test cases still pass. It turned out to be a little bit of a bigger patch then I would like and in fact there's a little bit more work to convert classes over but i'd thought I should post this first. Summary of changes: New Classes ISetting: interface for which IOSetting now implements (IOSetting is in package 'io' and we needed a way of using it in 'interfaces') SettingManager: simple wrapper around a map which handles adding/getting ISetting objects ChemObjectIO: abstract base class for all reader's and writers. Looking through the reader and writer code there was a lot of repeat code for adding and remove listeners. There was actually already a IChemObjectIO interface which provided the listener management but there was no implementation. Centralising the listener management to a single class means we only need to maintain it in one place, I also needed a place to add the setting manager to ChemObjectIO was created. New Tests new test in IChemObjectTest for adding dynamic settings (check for non-null entries) Modified Classes Major Changes Removed listener management code from DefaultChemObjectReader, DefaultChemObjectWriter, IteratingChemObjectReader and DefaultEventChemObjectReader. These are all now sub-classes of the new abstract ChemObjectIO which is doing the listener and setting management. There were also some minor changes to the methods that access listeners due them now being encapsulated. IChemObjectIO - Added methods for setting management and provided a 'getListeners()' method to allow subclasses to access the managed listeners. In addition I deprecated addChemObjectIOListener(...)/removeChemObjectIOListener(...) and added addListener(...)/removeListener(...) which is far less verbose and simplifies the API. IteratingMDLReader - now uses the new setting management. Minor changes IOSetting - implements ISetting interface CustomWriter - extends ChemObjectIO Readers/writers that use IOSettings (Currently all working but I will convert these): CDKSourceCodeWriter ChemObjectIO CMLWriter Gaussian98Reader MDLReader MDLRXNV3000Reader MDLV2000Reader MDLV2000Writer MDLV3000Reader PDBReader PDBWriter SDFWriter SMILESWriter GaussianInputWriter Cheers, J ---------------------------------------------------------------------- >Comment By: Egon Willighagen (egonw) Date: 2012-03-21 23:15 Message: Yes, two options, whatever is quicker for you: 1. keep marked @deprecated, and file a bug to get them removed, ro 2. just remove them (which is fine in master) ---------------------------------------------------------------------- Comment By: John May (jwmay) Date: 2012-03-21 10:21 Message: Argh! Why does SF have no edit button! ...in the previous comment by change I meant: no usage so are in need removal. ---------------------------------------------------------------------- Comment By: John May (jwmay) Date: 2012-03-21 10:19 Message: Quick question on 2. So I should still flag as deprecated in this patch, the bug report is to indicate the old methods need changing? ---------------------------------------------------------------------- Comment By: John May (jwmay) Date: 2012-03-21 02:42 Message: Okay sounds good, will try and get the changes done today. J ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2012-03-21 02:00 Message: John, some initial comments: 0. great changelog! 1. thanx for using generics! 2. please port the patch to master, and file a bug report about removing the deprecated code (once all IO classes are ported to the new API). 3. for all new files (new interface, etc), please update the following header to start copyright at this year, and add your name. For example, for ISetting.java replace /* * Copyright (C) 2003-2007 The Chemistry Development Kit (CDK) project with /* * Copyright (C) 2012 John May <jw...@us...> 4. class JavaDoc should start with a description of functionality. The following information is available anyway, and should not be part of the class JavaDoc (in several classes): +/** + * ISetting - 20.03.2012 <br/> + * <p/> (The data you can add as @cdk.created 2012-03-20 ...) 5. please check missing periods at the end of JavaDoc sentences. Particular the period after the first sentence is important, as the JavaDoc utility uses that to determine the first sentence, which is used as method summary in the TOC of methods at the start of a JavaDoc HTML page. 6. (optional, or for future patch) Methods like the following should be replaced or deprecated: @@ -328,7 +323,11 @@ implements IChemObjectIOListener { * @param skip ignore error molecules continue reading */ public void setSkip(boolean skip){ - this.skip = skip; + try { + getSetting("skip").setSetting(Boolean.toString(skip)); + } catch (CDKException ex){ + logger.debug("Unable toset 'skip' setting: ", ex); + } } private String extractFieldData(String str) throws IOException { Catching exceptions like this, which you are kind of forced to now, is something we should not be doing... 7. SettingManager has a main() method. Should that remain there? Or move to a unit test? 8. The SettingManager has some unfinished JavaDoc, like: + * @param name + * @param <S> + * + * @return Otherwise, I like where this is going! Thanx! ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2012-03-20 23:06 Message: I will review this patch (not meaning that others should not). Looks very interesting! ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=3509417&group_id=20024 |