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

[Config] Make config items case-insensitive #220

Merged
merged 7 commits into from
Dec 9, 2020
Merged

[Config] Make config items case-insensitive #220

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
238883a
config items should not be case sensitive
evelyn-ys Sep 28, 2020
bd728f5
add comments
evelyn-ys Sep 28, 2020
2468f41
reimplement
evelyn-ys Sep 28, 2020
15e8d0b
fix pattern
evelyn-ys Oct 27, 2020
2f193a2
Use regex grouping
jiasli Dec 2, 2020
b8c48a2
Refine tests
jiasli Dec 2, 2020
23c4837
Add comments
jiasli Dec 2, 2020
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
6 changes: 3 additions & 3 deletions knack/config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,9 +109,9 @@ def sections(self):

def items(self, section):
import re
pattern = self.env_var_name(section, '.+')
candidates = [(k.split('_')[-1], os.environ[k], k) for k in os.environ if re.match(pattern, k)]
result = {c[0]: c for c in candidates}
pattern = self.env_var_name(section, '[0-9A-Z][0-9A-Z_]*')
Copy link
Member

@jiasli jiasli Dec 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This will match CLI_MYSECTION_T part in CLI_MYSECTION_Test_Option, defeating our purpose of ignoring invalid env options like CLI_MYSECTION_Test_Option. We should use fullmatch instead.

candidates = [(k.replace(self.env_var_name(section, ''), ''), os.environ[k], k) for k in os.environ if re.match(pattern, k)]
Copy link
Member

Choose a reason for hiding this comment

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

replace can cause unwanted effect if self.env_var_name(section, '') somehow appears in the middle of the env var. removeprefix is a better choice but it is only available since Python 3.9.

Better to use grouping of regex.

result = {c[0].lower(): c for c in candidates}
for config in self._config_file_chain if self.use_local_config else self._config_file_chain[-1:]:
try:
entries = config.items(section)
Expand Down
16 changes: 16 additions & 0 deletions tests/test_config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,22 @@ def test_getboolean_error(self):
with self.assertRaises(ValueError):
self.cli_config.getboolean(section, option)

def test_items_case_insensitive(self):
section = 'MYSECTION'
envOption = '{}_{}_{}'.format(CLIConfig._DEFAULT_CONFIG_ENV_VAR_PREFIX, section, 'TEST_OPTION')
envVaule = 'envValue'
configOption = 'test_option'
configValue = 'value'
Copy link
Member

Choose a reason for hiding this comment

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

The python convention is to use snake case.

Copy link

Choose a reason for hiding this comment

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

The python convention is to use snake case.

Agree


with mock.patch.dict('os.environ', {envOption: envVaule}):
self.cli_config.set_value(section, configOption, configValue)
items_result = self.cli_config.items(section)
# The items function is expected to
# return environment setting instead of
# returning both local config 'Option' and environment variable 'OPTION'
self.assertEqual(len(items_result), 1)
self.assertEqual(items_result[0]['value'], 'envValue')

def test_set_config_value(self):
self.cli_config.set_value('test_section', 'test_option', 'a_value')
config = configparser.ConfigParser()
Expand Down