Only create a section when we initialise #89
Conversation
Codecov Report
@@ Coverage Diff @@
## master #89 +/- ##
=========================================
- Coverage 75.57% 75.08% -0.5%
=========================================
Files 6 6
Lines 303 309 +6
=========================================
+ Hits 229 232 +3
- Misses 74 77 +3
Continue to review full report at Codecov.
|
onelogin_aws_cli/configuration.py
Outdated
| @@ -69,7 +72,7 @@ def section(self, section_name: str): | |||
| :return: | |||
| """ | |||
| if not self.has_section(section_name): | |||
| self.add_section(section_name) | |||
| return None | |||
There was a problem hiding this comment.
You need to check that section_name != self.default_section. has_section will return false for self.default_section and that's going to lead to weirdness.
There was a problem hiding this comment.
Oh... do we assume the defaults section always exists?
If we do not assume it always exists and it doesn't exist we end up in a case where we can not know if it exists.
There was a problem hiding this comment.
yeah, it's confusing, that's why I had to add that confusing has_defaults prop in the other PR.
Although please let me know if there's an easier way to approach it.
|
Agree this makes things much cleaner, thanks. |
|
I stole some of your code, I hope you do not mind, but there will probably be merge conflicts |
drewsonne
force-pushed
the
feature/explicit-section-create
branch
from
f46ce5f
to
63b73d5
Compare
onelogin_aws_cli/configuration.py
Outdated
|
|
||
| self.file = config_file | ||
|
|
||
| if self.file is not None: | ||
| self.load() | ||
|
|
||
| @property |
There was a problem hiding this comment.
Technically not needed anymore (at least in this PR) =)
| if not self.has_section(section_name): | ||
| self.add_section(section_name) | ||
| section_missing = not self.has_section(section_name) | ||
| not_default = self.default_section != section_name |
There was a problem hiding this comment.
Could probably use some coverage for this case, but your other PR will probably cover it just by calling login without -C
There was a problem hiding this comment.
Yes, I was thinking that when the test cases passed after I modified it
onelogin_aws_cli/configuration.py
Outdated
| def __init__(self, config_file=None): | ||
| super().__init__(default_section='defaults') | ||
|
|
||
| self['defaults'] = dict(save_password=False) | ||
| self[self.default_section] = self.DEFAULTS |
There was a problem hiding this comment.
The reason I added the dict is to create a copy (in case changes in to the defaults across multiple instantiations in the unittests cross-pollute)
There was a problem hiding this comment.
cool, I rolled that commit back
There was a problem hiding this comment.
I have to come back to the test case another day, so we can either wait, or I'll create an issue to add the new test case.
There was a problem hiding this comment.
We can just push this and worry about it in one of the other PRs =).
drewsonne
force-pushed
the
feature/explicit-section-create
branch
from
429547c
to
900f957
Compare
We should not create a section when reading/writing only if it does not exist.
We should only create a section if we are trying to modify/write to the config file and the section does not exist.