Change of arguments in ITreeFactory
Brought to you by:
korry
Menu
▾
▴
#16 Change of arguments in ITreeFactory
I have a suggestion for changing the ITreeFactory:
I can never remember which one is the readonly and which is the createnew argument in the create() method.
Changing the names of the arguments from data to their proper names has two advantages:
a) help(ITreeFactory.create) returns something sensible
b) You can make use of pythons positional arguments
TreeFactory.create('file.aida', readOnly=True)
I also copied the docstring from the java file.
I was already thinking of a way to dynamically extract all the java docstrings from the java source (maybe with jython) and putting them in the python source, but didn't really have the time to follow through. It's also more complicated than I thought at first.
But Pythons introspection should in principle make it possible. The inspect module returns the source of a method...
Anyways, what do you think of the following:
Index: paida/paida_core/ITreeFactory.py
RCS file: /cvsroot/paida/paida/paida/paida_core/ITreeFactory.py,v
retrieving revision 1.10
diff -r1.10 ITreeFactory.py
16a17,28
> """ The creator of trees """
> def create(self, storeName=None, storeType='xml', readOnly=False, createNew=True, options=None):
> """ Creates a new tree and associates it with a store. The store is assumed to be read/write. The store will be created if it does not exist.
> Parameters:
> storeName - The name of the store, if empty (""), the tree is created in memory and therefore will not be associated with a file.
> storeType - Implementation specific string, may control store type
> readOnly - If true the store is opened readonly, an exception if it does not exist
> createNew - If false the file must exist, if true the file will be created
> options - Other options, currently are not specified
> Throws:
> IOException - if the store already exists
> IllegalArgumentException
18c30
< def create(self, data1 = None, data2 = None, data3 = None, data4 = None, data5 = None):
---
> """
20,22c32,34
< if (data1 == None) and (data2 == None) and (data3 == None) and (data4 == None) and (data5 == None):
< storeName = None
< storeType = 'xml'
---
> # if no storename is given, we need new defaults.
> # read-only mode and don't create a new file
> if storeName is None:
25,56c37,40
< option = optionAnalyzer(None)
< elif (type(data1) in types.StringTypes) and (data2 == None) and (data3 == None) and (data4 == None) and (data5 == None):
< storeName = data1
< storeType = 'xml'
< readOnly = False
< createNew = True
< option = optionAnalyzer(None)
< elif (type(data1) in types.StringTypes) and (type(data2) in types.StringTypes) and (data3 == None) and (data4 == None) and (data5 == None):
< storeName = data1
< storeType = data2.lower()
< readOnly = False
< createNew = True
< option = optionAnalyzer(None)
< elif (type(data1) in types.StringTypes) and (type(data2) in types.StringTypes) and (data3 in [True, False]) and (data4 == None) and (data5 == None):
< storeName = data1
< storeType = data2.lower()
< readOnly = data3
< createNew = True
< option = optionAnalyzer(None)
< elif (type(data1) in types.StringTypes) and (type(data2) in types.StringTypes) and (data3 in [True, False]) and (data4 in [True, False]) and (data5 == None):
< storeName = data1
< storeType = data2.lower()
< readOnly = data3
< createNew = data4
< option = optionAnalyzer(None)
< elif (type(data1) in types.StringTypes) and (type(data2) in types.StringTypes) and (data3 in [True, False]) and (data4 in [True, False]) and (type(data5) in types.StringTypes):
< storeName = data1
< storeType = data2.lower()
< readOnly = data3
< createNew = data4
< option = optionAnalyzer(data5)
< else:
---
> storeType = storeType.lower()
> options = optionAnalyzer(options)
> # type-checking the arguments
> if not (type(storeName) in types.StringTypes and type(storeType) in types.StringTypes and readOnly in [True, False] and createNew in [True, False] and type(options) in types.StringTypes):
74c58
< return ITree(storeName, storeType, readOnly, option)
---
> return ITree(storeName, storeType, readOnly, options)
Logged In: YES
user_id=1366327
Originator: YES
New diff; The one I just posted introduces a bug, because at the time of checking, the options are already converted to a dict.
I removed them from the check. The optionsAnalyzer can take care of the checking.
Index: paida/paida_core/ITreeFactory.py
RCS file: /cvsroot/paida/paida/paida/paida_core/ITreeFactory.py,v
retrieving revision 1.10
diff -r1.10 ITreeFactory.py
16a17,28
> """ The creator of trees """
> def create(self, storeName=None, storeType='xml', readOnly=False, createNew=True, options=None):
> """ Creates a new tree and associates it with a store. The store is assumed to be read/write. The store will be created if it does not exist.
> Parameters:
> storeName - The name of the store, if empty (""), the tree is created in memory and therefore will not be associated with a file.
> storeType - Implementation specific string, may control store type
> readOnly - If true the store is opened readonly, an exception if it does not exist
> createNew - If false the file must exist, if true the file will be created
> options - Other options, currently are not specified
> Throws:
> IOException - if the store already exists
> IllegalArgumentException
18c30
< def create(self, data1 = None, data2 = None, data3 = None, data4 = None, data5 = None):
---
> """
20,22c32,34
< if (data1 == None) and (data2 == None) and (data3 == None) and (data4 == None) and (data5 == None):
< storeName = None
< storeType = 'xml'
---
> # if no storename is given, we need new defaults.
> # read-only mode and don't create a new file
> if storeName is None:
25,56c37,40
< option = optionAnalyzer(None)
< elif (type(data1) in types.StringTypes) and (data2 == None) and (data3 == None) and (data4 == None) and (data5 == None):
< storeName = data1
< storeType = 'xml'
< readOnly = False
< createNew = True
< option = optionAnalyzer(None)
< elif (type(data1) in types.StringTypes) and (type(data2) in types.StringTypes) and (data3 == None) and (data4 == None) and (data5 == None):
< storeName = data1
< storeType = data2.lower()
< readOnly = False
< createNew = True
< option = optionAnalyzer(None)
< elif (type(data1) in types.StringTypes) and (type(data2) in types.StringTypes) and (data3 in [True, False]) and (data4 == None) and (data5 == None):
< storeName = data1
< storeType = data2.lower()
< readOnly = data3
< createNew = True
< option = optionAnalyzer(None)
< elif (type(data1) in types.StringTypes) and (type(data2) in types.StringTypes) and (data3 in [True, False]) and (data4 in [True, False]) and (data5 == None):
< storeName = data1
< storeType = data2.lower()
< readOnly = data3
< createNew = data4
< option = optionAnalyzer(None)
< elif (type(data1) in types.StringTypes) and (type(data2) in types.StringTypes) and (data3 in [True, False]) and (data4 in [True, False]) and (type(data5) in types.StringTypes):
< storeName = data1
< storeType = data2.lower()
< readOnly = data3
< createNew = data4
< option = optionAnalyzer(data5)
< else:
---
> storeType = storeType.lower()
> options = optionAnalyzer(options)
> # type-checking the arguments
> if not (type(storeName) in types.StringTypes and type(storeType) in types.StringTypes and readOnly in [True, False] and createNew in [True, False]):
74c58
< return ITree(storeName, storeType, readOnly, option)
---
> return ITree(storeName, storeType, readOnly, options)
Logged In: YES
user_id=1366327
Originator: YES
Actually, there's one more thing.
I think the default for readonly should be true
This is the default in python for opening a file and ensures data integrity.
I have already been bitten a couple time by thinking I create a new file and instead I added data to an existing file.
Cheers,
Jan
Logged In: YES
user_id=1366327
Originator: YES
Hi Koji,
sorry, last patch, I hope.
There's a bug in the creation of the tree.
CreateNew must always create a new file, but it doesn't.
Try this.
Index: ITreeFactory.py
RCS file: /cvsroot/paida/paida/paida/paida_core/ITreeFactory.py,v
retrieving revision 1.10
diff -r1.10 ITreeFactory.py
16a17,28
> """ The creator of trees """
> def create(self, storeName=None, storeType='xml', readOnly=False, createNew=True, options=None):
> """ Creates a new tree and associates it with a store. The store is assumed to be read/write. The store will be created if it does not exist.
> Parameters:
> storeName - The name of the store, if empty (""), the tree is created in memory and therefore will not be associated with a file.
> storeType - Implementation specific string, may control store type
> readOnly - If true the store is opened readonly, an exception if it does not exist
> createNew - If false the file must exist, if true the file will be created
> options - Other options, currently are not specified
> Throws:
> IOException - if the store already exists
> IllegalArgumentException
18c30
< def create(self, data1 = None, data2 = None, data3 = None, data4 = None, data5 = None):
---
> """
20,22c32,34
< if (data1 == None) and (data2 == None) and (data3 == None) and (data4 == None) and (data5 == None):
< storeName = None
< storeType = 'xml'
---
> # if no storename is given, we need new defaults.
> # read-only mode and don't create a new file
> if storeName is None:
25,56c37,40
< option = optionAnalyzer(None)
< elif (type(data1) in types.StringTypes) and (data2 == None) and (data3 == None) and (data4 == None) and (data5 == None):
< storeName = data1
< storeType = 'xml'
< readOnly = False
< createNew = True
< option = optionAnalyzer(None)
< elif (type(data1) in types.StringTypes) and (type(data2) in types.StringTypes) and (data3 == None) and (data4 == None) and (data5 == None):
< storeName = data1
< storeType = data2.lower()
< readOnly = False
< createNew = True
< option = optionAnalyzer(None)
< elif (type(data1) in types.StringTypes) and (type(data2) in types.StringTypes) and (data3 in [True, False]) and (data4 == None) and (data5 == None):
< storeName = data1
< storeType = data2.lower()
< readOnly = data3
< createNew = True
< option = optionAnalyzer(None)
< elif (type(data1) in types.StringTypes) and (type(data2) in types.StringTypes) and (data3 in [True, False]) and (data4 in [True, False]) and (data5 == None):
< storeName = data1
< storeType = data2.lower()
< readOnly = data3
< createNew = data4
< option = optionAnalyzer(None)
< elif (type(data1) in types.StringTypes) and (type(data2) in types.StringTypes) and (data3 in [True, False]) and (data4 in [True, False]) and (type(data5) in types.StringTypes):
< storeName = data1
< storeType = data2.lower()
< readOnly = data3
< createNew = data4
< option = optionAnalyzer(data5)
< else:
---
> storeType = storeType.lower()
> options = optionAnalyzer(options)
> # type-checking the arguments
> if not (type(storeName) in types.StringTypes and type(storeType) in types.StringTypes and readOnly in [True, False] and createNew in [True, False]):
67,74c51,57
< if not os.path.exists(storeName):
< if createNew:
< storeFile = file(storeName, 'w')
< storeFile.write(_initialXml)
< storeFile.close()
< else:
< raise IOException('%s does not exist.' % storeName)
< return ITree(storeName, storeType, readOnly, option)
---
> if createNew:
> storeFile = open(storeName, 'w')
> storeFile.write(_initialXml)
> storeFile.close()
> elif not os.path.exists(storeName):
> raise IOException('%s does not exist.' % storeName)
> return ITree(storeName, storeType, readOnly, options)
ITreeFactory patch
Logged In: YES
user_id=1366327
Originator: YES
OK, I should really test more before submitting patches.
The last patch breaks the default arguments, so that ITreeFactory.create() didn't work any more.
Rather than spamming the page, I'll upload the file...
File Added: ITreeFactory.patch
Logged In: YES
user_id=734761
Originator: NO
Hi Jan,
Thanks for the series of patches!
The last one seems to work correctly. I've checked it into CVS.
Logged In: YES
user_id=1366327
Originator: YES
Great !
Thanks, Koji. Sorry it took me a while to get it right.
Do you think it's time for a new release?