[tor-bugs] #4913 [Stem]: Add stem.util.conf.Config.save()

[Tor Bug Tracker & Wiki]

#4913: Add stem.util.conf.Config.save()
-------------------------+--------------------------------------------------
 Reporter:  gsathya      |          Owner:  atagar        
     Type:  enhancement  |         Status:  needs_revision
 Priority:  normal       |      Milestone:                
Component:  Stem         |        Version:                
 Keywords:               |         Parent:                
   Points:               |   Actualpoints:                
-------------------------+--------------------------------------------------

Comment(by atagar):

 > Abiding by the coding guidelines of our benevolent dictator

 Heh

 > commit 6185b825701025ed7ca371bedb3f0d2b1b7e9697
 > -  def test_example(self):
 > +  def test_save_example(self):

 Please leave the current test_example() test alone and instead just add a
 new one. That test is to confirm that the example in the pydoc actually
 works.

 That said, feel free to refactor if there's commonalities and it stays
 faithful to the what's in the doc for the Config class.

 > +  def makeConf(self, path, content):
 > +    self.tearDown()
 > +    with open(path, 'w') as output_file:
 > +      output_file.write(content)

 Hmm, few nit picks...

 - Method and function names use the underscore convention (in this case
 "make_conf"). I did camel case like that in arm but switched for stem
 because underscores are both more readable and the 'official' coding
 contention in python.

 From "http://www.python.org/dev/peps/pep-0008/"...
 "Function names should be lowercase, with words separated by underscores
 as necessary to improve readability."

 Also, python's convention is that private functions start with an
 underscore so this should actually be "_make_conf".

 - This really shouldn't be calling tearDown() directly - both setUp() and
 tearDown() are methods of the unit test class that are executed before and
 after the tests respectively. And yes, I realize that they don't follow
 the underscore convention. ;)

 > commit e154c99311b1a3a05bd6b633d6d4b3b7823e9487
 > +    user_config.save()
 > +    user_config.clear()
 > +    user_config.load(CONF_PATH)

 Great if it didn't disrupt the test_example(). The way I usually approach
 writing tests is to first write them as stand-alone functions (even if
 this duplicates code), then refactor out repetitive bits afterward. In
 testing code repetitions are a bit more acceptable as long as the tests
 are readable.

 > +  def test_save_empty(self):

 Hmm, this looks more like a test_save_example test. What I meant by 'save
 empty' was to call save with an empty configuration, like...

 user_config = stem.util.conf.get_config("integ-save_empty")
 user_config.save(test_path)

 # assert that file exists and that it's empty

 user_config.load(test_path)

 # assert that user_config is still empty

 Speaking of which, the save() function should probably accept an optional
 path (and if not provided use self._path).

 > +    self.tearDown()

 You don't need to call tearDown() explicitly - it's automatically called
 after the test.

 > commit 56e76d1f206639d15d6e2de7e17732753debc112
 > +      for entry_key in sorted(self.keys()):

 Perfect

 Cheers! -Damian

-- 
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/4913#comment:12>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online