RecursionError in LocalFileStorage

[lilyminium]

openff-evaluator/openff/evaluator/storage/localfile.py

Lines 18 to 21 in 64d7440

	@property
	def root_directory(self):
	"""str: Returns the directory in which all stored objects are located."""
	return self.root_directory

This recursive call means the property is not really very helpful.

>>> from openff.evaluator.storage import LocalFileStorage
>>> lfs = LocalFileStorage()
>>> lfs.root_directory
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/lily/anaconda3/envs/polymetrizer/lib/python3.9/site-packages/openff/evaluator/storage/localfile.py", line 21, in root_directory
    return self.root_directory
  File "/Users/lily/anaconda3/envs/polymetrizer/lib/python3.9/site-packages/openff/evaluator/storage/localfile.py", line 21, in root_directory
    return self.root_directory
  File "/Users/lily/anaconda3/envs/polymetrizer/lib/python3.9/site-packages/openff/evaluator/storage/localfile.py", line 21, in root_directory
    return self.root_directory
  [Previous line repeated 996 more times]
RecursionError: maximum recursion depth exceeded

My suggestion would be to just not privatize attributes with getter/setters, as I think it adds burden to code maintenance and is liable to bugs such as the one reported here. (The actual attribute used is _root_directory.)

[SimonBoothroyd]

My suggestion would be to just not privatize attributes with getter/setters, as I think it adds burden to code maintenance and is liable to bugs such as the one reported here. (The actual attribute used is _root_directory.)

This is a pretty common code pattern and gives a bit of safeguarding in case you later change the internal code to not be resilient to external changes of this variable - I think the main issue here is bad testing and not necessarily maintenance burden!

A PR containing the typo fix + test would be welcome 🙂

[lilyminium]

This is a pretty common code pattern and gives a bit of safeguarding in case you later change the internal code

Sure, I suppose I am a fervent follower of YAGNI and think it's fairly easy to change an attribute to a managed property -- if anything, this kind of code encourages me to use the "private" attribute because the public one can't really be used -- but we all have different styles :-)

I think the main issue here is bad testing and not necessarily maintenance burden!

I think of needing to write and maintain additional tests as maintenance burden!