-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Replace hints.uppercase with hints.labels #7661
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -879,6 +879,40 @@ def test_content_media_capture(self, yaml, autoconfig): | |
'content.media.video_capture']: | ||
assert data[setting]['global'] == val | ||
|
||
@pytest.mark.parametrize('chars, uppercase', [ | ||
('aoeuidnths', True), | ||
('abcdefghijklmnopqrstuvwxyz', True), | ||
(None, True), | ||
('aoeuidnths', False), | ||
('abcdefghijklmnopqrstuvwxyz', False), | ||
(None, False), | ||
('aoeuidnths', None), | ||
('abcdefghijklmnopqrstuvwxyz', None), | ||
(None, None), | ||
]) | ||
def test_hints_uppercase(self, yaml, autoconfig, chars, uppercase): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not super happy with this test. I dislike the use |
||
settings = {} | ||
if chars is not None: | ||
settings['hints.chars'] = {'global': chars} | ||
if uppercase is not None: | ||
settings['hints.uppercase'] = {'global': uppercase} | ||
|
||
autoconfig.write(settings) | ||
|
||
yaml.load() | ||
yaml._save() | ||
|
||
data = autoconfig.read() | ||
if uppercase: | ||
if 'hints.chars' in data: | ||
labels = data['hints.chars']['global'].upper() | ||
assert data['hints.labels']['global'] == labels | ||
else: | ||
labels = configdata.DATA['hints.chars'].default.upper() | ||
assert data['hints.labels']['global'] == labels | ||
else: | ||
assert 'hints.labels' not in data | ||
|
||
def test_empty_pattern(self, yaml, autoconfig): | ||
valid_pattern = 'https://example.com/*' | ||
invalid_pattern = '*://*./*' | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the description is sufficient here. I only understand it because I have read the discussion leading to this feature. Also, I would assume that most users do not use a
config.py
and setting one up to preserve the previous functionality seems a bit too much IMHO. Couldn't we use some special value that would be interpreted ashints.chars.upper()
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
People can always just set it to
ASDFGHJKL
(assuminghints.chars
set toasdfghjkl
) - in fact, that's what the config migration does too!Yeah, it's a bit cumbersome because you need to change things at two places - but at the same time, having a special
<upper>
value or whatever seems weird to me as well.As for the description, what would you suggest? Would something like a "For every character in
hints.chars
, the character of this setting at the same position will be shown as the corresponding hint text." or so suffice?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah maybe the description could be more concrete. I think the line proposed by @The-Compiler is good and descriptive, but I'll throw my hat into the ring: "In hint text, characters of this settings will be shown instead of characters of
hints.chars
." A little less descriptive, but a bit more concise.