#16 Change of arguments in ITreeFactory

Improvement
open
Core (6)
5
2007-02-08
2007-02-03
Jan Strube
No

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)

Discussion

  • Jan Strube

    Jan Strube - 2007-02-03

    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)

     
  • Jan Strube

    Jan Strube - 2007-02-04

    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

     
  • Jan Strube

    Jan Strube - 2007-02-04

    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)

     
  • Jan Strube

    Jan Strube - 2007-02-08

    ITreeFactory patch

     
  • Jan Strube

    Jan Strube - 2007-02-08

    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

     
  • Koji Kishimoto

    Koji Kishimoto - 2007-02-08
    • labels: --> Core
    • milestone: --> Improvement
    • assigned_to: nobody --> korry
     
  • Koji Kishimoto

    Koji Kishimoto - 2007-02-08

    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.

     
  • Jan Strube

    Jan Strube - 2007-02-08

    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?

     

Log in to post a comment.

Get latest updates about Open Source Projects, Conferences and News.

Sign up for the SourceForge newsletter:

JavaScript is required for this form.





No, thanks