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.

Sign up for GitHub

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

Jump to bottom

Fix error with LuigiTomlParser usage in S3Client #3076

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Fix error with LuigiTomlParser usage in S3Client #3076

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fd6f9c0
General usage of both LuigiConfigParser and LuigiTomlParser in S3 client
junhl Apr 19, 2021
869d38d
Add lacking functions to LuigiTomlParser to match LuigiConfigParser
junhl Apr 19, 2021
924100d
Merge branch 'master' into fix-bug-toml-s3-client
junhl Apr 19, 2021
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
17 changes: 17 additions & 0 deletions luigi/configuration/toml_parser.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,23 @@ def read(self, config_paths):

return self.data

def defaults(self):
Copy link
Author

@junhl junhl Apr 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

solution 1: fill lacking functions to LuigiTomlParser. This is just dummy function so it's not my preferred one.
(will add tests if preferred by reviewers)

"""
Unlike ConfigParser, [DEFAULT] section is not particularly meaningful in toml.
This is to match LuigiConfigParser.
"""
return {}

def items(self, section=None):
"""
Return a list of (name, value) tuples for each option in a section.
This is to match LuigiConfigParser.
"""
if section is None:
return self.data.items()
else:
return [(option, value) for option, value in self.data[section].items()]

def get(self, section, option, default=NO_DEFAULT, **kwargs):
try:
return self.data[section][option]
Expand Down
7 changes: 5 additions & 2 deletions luigi/contrib/s3.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -480,9 +480,12 @@ def list(self, path, start_time=None, end_time=None, return_key=False): # backw

@staticmethod
def _get_s3_config(key=None):
defaults = dict(configuration.get_config().defaults())
# LuigiConfigParser has defaults since [DEFAULT] is a special section
# LuigiTomlParser does not have it because [DEFAULT] is not a special section
luigi_config = configuration.get_config()
defaults = dict(luigi_config.defaults()) if hasattr(luigi_config, "defaults") else {}
try:
config = dict(configuration.get_config().items('s3'))
config = dict(luigi_config['s3'])
Copy link
Author

@junhl junhl Apr 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

solution 2: change usage to generic one to use existing __getitem__ and only take defaults when it's meaningful (ex. LuigiConfigParser) instead of creating dummy function. (my pref. Will add tests if reviewers also think so)

except (NoSectionError, KeyError):
return {}
# So what ports etc can be read without us having to specify all dtypes
Expand Down