-
Notifications
You must be signed in to change notification settings - Fork 280
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
Key ordering with locale, take #2 #288
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 |
---|---|---|
|
@@ -95,6 +95,13 @@ def setUpClass(cls): | |
# dos line endings yaml | ||
'dos.yml': '---\r\n' | ||
'dos: true', | ||
# different key-ordering by locale | ||
'c.yaml': '---\n' | ||
'A: true\n' | ||
'a: true', | ||
'en.yaml': '---\n' | ||
'a: true\n' | ||
'A: true' | ||
}) | ||
|
||
@classmethod | ||
|
@@ -108,8 +115,10 @@ def test_find_files_recursively(self): | |
self.assertEqual( | ||
sorted(cli.find_files_recursively([self.wd], conf)), | ||
[os.path.join(self.wd, 'a.yaml'), | ||
os.path.join(self.wd, 'c.yaml'), | ||
os.path.join(self.wd, 'dos.yml'), | ||
os.path.join(self.wd, 'empty.yml'), | ||
os.path.join(self.wd, 'en.yaml'), | ||
os.path.join(self.wd, 's/s/s/s/s/s/s/s/s/s/s/s/s/s/s/file.yaml'), | ||
os.path.join(self.wd, 'sub/directory.yaml/empty.yml'), | ||
os.path.join(self.wd, 'sub/ok.yaml'), | ||
|
@@ -146,6 +155,8 @@ def test_find_files_recursively(self): | |
self.assertEqual( | ||
sorted(cli.find_files_recursively([self.wd], conf)), | ||
[os.path.join(self.wd, 'a.yaml'), | ||
os.path.join(self.wd, 'c.yaml'), | ||
os.path.join(self.wd, 'en.yaml'), | ||
os.path.join(self.wd, 's/s/s/s/s/s/s/s/s/s/s/s/s/s/s/file.yaml'), | ||
os.path.join(self.wd, 'sub/ok.yaml'), | ||
os.path.join(self.wd, 'warn.yaml')] | ||
|
@@ -175,8 +186,10 @@ def test_find_files_recursively(self): | |
self.assertEqual( | ||
sorted(cli.find_files_recursively([self.wd], conf)), | ||
[os.path.join(self.wd, 'a.yaml'), | ||
os.path.join(self.wd, 'c.yaml'), | ||
os.path.join(self.wd, 'dos.yml'), | ||
os.path.join(self.wd, 'empty.yml'), | ||
os.path.join(self.wd, 'en.yaml'), | ||
os.path.join(self.wd, 'no-yaml.json'), | ||
os.path.join(self.wd, 'non-ascii/éçäγλνπ¥/utf-8'), | ||
os.path.join(self.wd, 's/s/s/s/s/s/s/s/s/s/s/s/s/s/s/file.yaml'), | ||
|
@@ -194,8 +207,10 @@ def test_find_files_recursively(self): | |
self.assertEqual( | ||
sorted(cli.find_files_recursively([self.wd], conf)), | ||
[os.path.join(self.wd, 'a.yaml'), | ||
os.path.join(self.wd, 'c.yaml'), | ||
os.path.join(self.wd, 'dos.yml'), | ||
os.path.join(self.wd, 'empty.yml'), | ||
os.path.join(self.wd, 'en.yaml'), | ||
os.path.join(self.wd, 'no-yaml.json'), | ||
os.path.join(self.wd, 'non-ascii/éçäγλνπ¥/utf-8'), | ||
os.path.join(self.wd, 's/s/s/s/s/s/s/s/s/s/s/s/s/s/s/file.yaml'), | ||
|
@@ -315,6 +330,46 @@ def test_run_with_user_yamllint_config_file_in_env(self): | |
cli.run((os.path.join(self.wd, 'a.yaml'), )) | ||
self.assertEqual(ctx.returncode, 1) | ||
|
||
def test_run_with_locale(self): | ||
# check for availability of locale, otherwise skip the test | ||
# reset to default before running the test, | ||
# as the first two runs don't use setlocale() | ||
try: | ||
locale.setlocale(locale.LC_ALL, 'en_US.UTF-8') | ||
except locale.Error: | ||
self.skipTest('locale en_US.UTF-8 not available') | ||
locale.setlocale(locale.LC_ALL, (None, None)) | ||
|
||
# C + en.yaml should fail | ||
with RunContext(self) as ctx: | ||
cli.run(('-d', 'rules: { key-ordering: enable }', | ||
os.path.join(self.wd, 'en.yaml'))) | ||
self.assertEqual(ctx.returncode, 1) | ||
|
||
# C + c.yaml should pass | ||
with RunContext(self) as ctx: | ||
cli.run(('-d', 'rules: { key-ordering: enable }', | ||
os.path.join(self.wd, 'c.yaml'))) | ||
self.assertEqual(ctx.returncode, 0) | ||
|
||
# the next two runs use setlocale() inside, | ||
# so we need to clean up afterwards | ||
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. 👍 |
||
self.addCleanup(locale.setlocale, locale.LC_ALL, (None, None)) | ||
|
||
# en_US + en.yaml should pass | ||
with RunContext(self) as ctx: | ||
cli.run(('-d', 'locale: en_US.UTF-8\n' | ||
'rules: { key-ordering: enable }', | ||
os.path.join(self.wd, 'en.yaml'))) | ||
self.assertEqual(ctx.returncode, 0) | ||
|
||
# en_US + c.yaml should fail | ||
with RunContext(self) as ctx: | ||
cli.run(('-d', 'locale: en_US.UTF-8\n' | ||
'rules: { key-ordering: enable }', | ||
os.path.join(self.wd, 'c.yaml'))) | ||
self.assertEqual(ctx.returncode, 1) | ||
|
||
def test_run_version(self): | ||
with RunContext(self) as ctx: | ||
cli.run(('--version', )) | ||
|
@@ -375,12 +430,11 @@ def test_run_non_ascii_file(self): | |
|
||
# Make sure the default localization conditions on this "system" | ||
# support UTF-8 encoding. | ||
loc = locale.getlocale() | ||
try: | ||
locale.setlocale(locale.LC_ALL, 'C.UTF-8') | ||
locale.setlocale(locale.LC_ALL, (None, 'UTF-8')) | ||
except locale.Error: | ||
locale.setlocale(locale.LC_ALL, 'en_US.UTF-8') | ||
self.addCleanup(locale.setlocale, locale.LC_ALL, loc) | ||
self.skipTest('no UTF-8 locale available') | ||
self.addCleanup(locale.setlocale, locale.LC_ALL, (None, None)) | ||
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 now understand more how the whole 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. Interesting: Thanks! |
||
|
||
with RunContext(self) as ctx: | ||
cli.run(('-f', 'parsable', path)) | ||
|
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.
Can you briefly explain why you added this? Is the cleanup on line 334 redundant?
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.
The cleanup is still needed after the whole test ran.
Setting the locale to
en_US
is just a test for locale availability. But because the default without locale option (as in the first of the for test.run
s below) doesn't set any locale, thaten_US
would still be active. Therefore we need to reset that to it's initial state first, then run the 2 tests with default settings, then the 2 with locale setting, then cleanup.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.
Thanks!
I understood the second one (line 339), but I thought the cleanup wasn't needed, then. Actually it's still needed because yamllint calls
locale.setlocale()
too (incli.py
).Could you add a tiny comment above
self.addCleanup()
, just to make sure we remember it? E.g.# Clean up after yamllint called locale.setlocale() in cli.py:
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 added some comments and moved the
addCleanup
to a more appropriate spot regarding the flow in this test.