Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lazy filesystem operations at FileConfig #113

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 34 additions & 15 deletions datadotworld/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ class DefaultConfig(object):
def __init__(self):
self._auth_token = None
self._tmp_dir = path.expanduser(tempfile.gettempdir())
self._cache_dir = path.expanduser('~/.dw/cache')
self._cache_dir = None

@property
def auth_token(self):
Expand Down Expand Up @@ -88,16 +88,33 @@ class FileConfig(DefaultConfig):
:param profile: Name of configuration profile.
:type profile: str
"""
_config_parser = None

def __init__(self, profile='default', **kwargs):
super(FileConfig, self).__init__()

# Overrides, for testing
self._config_file_path = path.expanduser(
kwargs.get('config_file_path', '~/.dw/config'))
legacy_file_path = path.expanduser(
self.legacy_file_path = path.expanduser(
kwargs.get('legacy_file_path', '~/.data.world'))

self._profile = profile
self._section = (profile
if profile.lower() != configparser.DEFAULTSECT.lower()
else configparser.DEFAULTSECT)

self._cache_dir = path.expanduser('~/.dw/cache')

@property
def config_parser(self):
if self._config_parser is None:
self._config_parser = self.get_config_parser()

return self._config_parser

def get_config_parser(self):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the interest of consistency, would you mind adding a dunder to this method name since it's private?

Suggested change
def get_config_parser(self):
def __get_config_parser(self):

# FIXME this should probably be a pure function
if not path.isdir(path.dirname(self._config_file_path)):
os.makedirs(path.dirname(self._config_file_path))

Expand All @@ -106,24 +123,23 @@ def __init__(self, profile='default', **kwargs):

if path.isfile(self._config_file_path):
self._config_parser.read_file(open(self._config_file_path))

if self.__migrate_invalid_defaults(self._config_parser) > 0:
self.save()
elif path.isfile(legacy_file_path):
self._config_parser = self.__migrate_config(legacy_file_path)
self.save()

self._profile = profile
self._section = (profile
if profile.lower() != configparser.DEFAULTSECT.lower()
else configparser.DEFAULTSECT)
elif path.isfile(self.legacy_file_path):
self._config_parser = self.__migrate_config(self.legacy_file_path)
self.save()

if not path.isdir(path.dirname(self.cache_dir)):
os.makedirs(path.dirname(self.cache_dir))

return self._config_parser

@property
def auth_token(self):
self.__validate_config()
return self._config_parser.get(self._section, 'auth_token')
return self.config_parser.get(self._section, 'auth_token')

@auth_token.setter
def auth_token(self, auth_token):
Expand All @@ -133,22 +149,25 @@ def auth_token(self, auth_token):

"""
if (self._section != configparser.DEFAULTSECT and
not self._config_parser.has_section(self._section)):
self._config_parser.add_section(self._section)
self._config_parser.set(self._section, 'auth_token', auth_token)
not self.config_parser.has_section(self._section)):
self.config_parser.add_section(self._section)
self.config_parser.set(self._section, 'auth_token', auth_token)

def save(self):
"""Persist config changes"""
with open(self._config_file_path, 'w') as file:
self._config_parser.write(file)
self.config_parser.write(file)

def __validate_config(self):
config_parser = self.config_parser
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels unnecessary since it's only ever called once in this method; how about just calling self.config_parser down there?


if not path.isfile(self._config_file_path):
raise RuntimeError(
'Configuration file not found at {}.'
'To fix this issue, run dw configure'.format(
self._config_file_path))
if not self._config_parser.has_option(self._section, 'auth_token'):

if not config_parser.has_option(self._section, 'auth_token'):
raise RuntimeError(
'The {0} profile is not properly configured. '
'To fix this issue, run dw -p {0} configure'.format(
Expand Down
16 changes: 14 additions & 2 deletions tests/datadotworld/test_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ def test_auth_token(self):

def test_cache_dir(self):
assert_that(DefaultConfig().cache_dir,
equal_to(path.expanduser('~/.dw/cache')))
equal_to(None))

def test_tmp_dir(self):
assert_that(DefaultConfig().tmp_dir,
Expand All @@ -75,7 +75,7 @@ def test_auth_token(self):
def test_cache_dir(self):
config = InlineConfig('inline_token')
assert_that(config.cache_dir,
equal_to(path.expanduser('~/.dw/cache')))
equal_to(None))

def test_tmp_dir(self):
config = InlineConfig('inline_token')
Expand Down Expand Up @@ -191,6 +191,18 @@ def test_save_overwrite(self, config_file_path):
config_reloaded = FileConfig(config_file_path=config_file_path)
assert_that(config_reloaded.auth_token, equal_to('newtoken'))

@pytest.mark.usefixtures('config_directory', 'default_config_file')
def test_invalid_config_file_path(self, config_file_path):
config = FileConfig(config_file_path='/foo/bar/baz/')

with pytest.raises(PermissionError):
config.get_config_parser()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The public accessor here is just config_parser since it's a property.

Suggested change
config.get_config_parser()
config.config_parser


def test_cache_dir(self):
config = FileConfig()
assert_that(config.cache_dir,
equal_to(path.expanduser('~/.dw/cache')))


class TestChainedConfig:
@pytest.fixture()
Expand Down