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

Fixed list parsing from ConfigIniEnv and adjusted code to match docs. #71

Merged
merged 1 commit into from Dec 13, 2018

Conversation

apollo13
Copy link
Contributor

@apollo13 apollo13 commented Nov 5, 2018

Hi there; this is a small patch (with tests) that brings the behavior of ConfigIniEnv in line with other variants. What do you think?

@apollo13
Copy link
Contributor Author

apollo13 commented Nov 5, 2018

If you deem this patch backwards compatible I can add a completely backwards compatible one which moves ConfigObj construction into a factory method for instance and keep the original behaviour as default.

@willkg
Copy link
Owner

willkg commented Nov 5, 2018

I'm kind of pressed for time and I haven't touched Everett in a while so I don't have it loaded into my head. Can you explain what this does specifically?

@apollo13
Copy link
Contributor Author

apollo13 commented Nov 5, 2018

Sure; basically it does two things:

  • It changes ConfigIniEnv to just load the first found file if passed a list instead of reading all (ie it aligns code with the documented behavior). [ "uses the first one it finds" as documented here ]
  • It changes ConfigIniEnv to return plain strings and not let configobj.ConfigObj parse those into lists to align the behavior of ConfigIniEnv with ConfigOSEnv and others which do not change values. This is important in the sense that it is hard to work with it if different configs return different datatypes. If one wants lists from the config, there is already ListOf which achieves that.


@classmethod
def parse_ini_file(cls, path):
Copy link
Owner

Choose a reason for hiding this comment

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

If this isn't a class method anymore, then the cls argument should be self.

Copy link
Owner

@willkg willkg left a comment

Choose a reason for hiding this comment

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

I'm really sorry this took me so long to look at. This looks super and I really appreciate you working on it!

I had one minor nit. I think I'll land this now and then fix the nit.

Thank you!

@willkg willkg merged commit 6e81e2f into willkg:master Dec 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants