add python formatting and checking with ruff (#6772)

* add python formatting and checking with ruff * Autoformat python files * address ruff fixes - remove unused modules / variables - fix tests to start with test_* so that they run, change tests so that they pass - remove duplicate definitions - use `x not in y` instead of `not x in y` --------- Co-authored-by: Ben Brooks <[email protected]>
couchbase · May 16, 2024 · 30d0291 · 30d0291
1 parent 154bee3
commit 30d0291
Show file tree

Hide file tree

Showing 8 changed files with 903 additions and 486 deletions.
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
@@ -111,7 +111,18 @@ jobs:
         uses: guyarb/[email protected]
         with:
           test-results: test.json
-
+  python-format:
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v4
+      - uses: chartboost/ruff-action@v1
+        with:
+          args: 'format --check'
+  python-lint:
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v4
+      - uses: chartboost/ruff-action@v1
   test-sgcollect:
     runs-on: ${{ matrix.os }}
     strategy:

diff --git a/tools-tests/conftest.py b/tools-tests/conftest.py
@@ -11,7 +11,9 @@
 import sys
 
 module_name = "sgcollect_info"
-spec = spec_from_loader(module_name, SourceFileLoader(module_name, "tools/" + module_name))
+spec = spec_from_loader(
+    module_name, SourceFileLoader(module_name, "tools/" + module_name)
+)
 mod = module_from_spec(spec)
 sys.modules[spec.name] = mod
 spec.loader.exec_module(mod)
diff --git a/tools-tests/parser_test.py b/tools-tests/parser_test.py
@@ -8,5 +8,6 @@
 
 import sgcollect_info
 
+
 def test_parser():
-    parser = sgcollect_info.create_option_parser()
+    sgcollect_info.create_option_parser()
diff --git a/tools-tests/password_remover_test.py b/tools-tests/password_remover_test.py
@@ -13,17 +13,16 @@
 
 import password_remover
 
-class TestStripPasswordsFromUrl(unittest.TestCase):
 
-    def basic_test(self):
+class TestStripPasswordsFromUrl(unittest.TestCase):
+    def test_basic(self):
         url_with_password = "http://bucket-1:foobar@localhost:8091"
-        url_no_password = strip_password_from_url(url_with_password)
+        url_no_password = password_remover.strip_password_from_url(url_with_password)
         assert "foobar" not in url_no_password
         assert "bucket-1" in url_no_password
 
 
 class TestRemovePasswords(unittest.TestCase):
-
     def test_basic(self):
         json_with_passwords = r"""
         {
@@ -64,7 +63,6 @@ def test_basic(self):
         assert "foobar" not in with_passwords_removed
 
     def test_alternative_config(self):
-
         sg_config = '{"Interface":":4984","AdminInterface":":4985","Facebook":{"Register":true},"Log":["*"],"Databases":{"todolite":{"server":"http://localhost:8091","pool":"default","bucket":"default","password":"foobar","name":"todolite","sync":"\\nfunction(doc, oldDoc) {\\n  // NOTE this function is the same across the iOS, Android, and PhoneGap versions.\\n  if (doc.type == \\"task\\") {\\n    if (!doc.list_id) {\\n      throw({forbidden : \\"Items must have a list_id.\\"});\\n    }\\n    channel(\\"list-\\"+doc.list_id);\\n  } else if (doc.type == \\"list\\" || (doc._deleted \\u0026\\u0026 oldDoc \\u0026\\u0026 oldDoc.type == \\"list\\")) {\\n    // Make sure that the owner propery exists:\\n    var owner = oldDoc ? oldDoc.owner : doc.owner;\\n    if (!owner) {\\n      throw({forbidden : \\"List must have an owner.\\"});\\n    }\\n\\n    // Make sure that only the owner of the list can update the list:\\n    if (doc.owner \\u0026\\u0026 owner != doc.owner) {\\n      throw({forbidden : \\"Cannot change owner for lists.\\"});\\n    }\\n\\n    var ownerName = owner.substring(owner.indexOf(\\":\\")+1);\\n    requireUser(ownerName);\\n\\n    var ch = \\"list-\\"+doc._id;\\n    if (!doc._deleted) {\\n      channel(ch);\\n    }\\n\\n    // Grant owner access to the channel:\\n    access(ownerName, ch);\\n\\n    // Grant shared members access to the channel:\\n    var members = !doc._deleted ? doc.members : oldDoc.members;\\n    if (Array.isArray(members)) {\\n      var memberNames = [];\\n      for (var i = members.length - 1; i \\u003e= 0; i--) {\\n        memberNames.push(members[i].substring(members[i].indexOf(\\":\\")+1))\\n      };\\n      access(memberNames, ch);\\n    }\\n  } else if (doc.type == \\"profile\\") {\\n    channel(\\"profiles\\");\\n    var user = doc._id.substring(doc._id.indexOf(\\":\\")+1);\\n    if (user !== doc.user_id) {\\n      throw({forbidden : \\"Profile user_id must match docid.\\"});\\n    }\\n    requireUser(user);\\n    access(user, \\"profiles\\");\\n  }\\n}\\n","users":{"GUEST":{"name":"","admin_channels":["*"],"all_channels":null,"disabled":true}}}}}'
 
         with_passwords_removed = password_remover.remove_passwords(sg_config)
@@ -125,12 +123,13 @@ def test_non_parseable_config(self):
           }
         }
         """
-        with_passwords_removed = password_remover.remove_passwords(unparseable_json_with_passwords)
+        with_passwords_removed = password_remover.remove_passwords(
+            unparseable_json_with_passwords
+        )
         assert "foobar" not in with_passwords_removed
 
 
 class TestTagUserData(unittest.TestCase):
-
     def test_basic(self):
         json_with_userdata = """
         {
@@ -155,17 +154,17 @@ def test_basic(self):
         """
         tagged = password_remover.tag_userdata_in_server_config(json_with_userdata)
         assert "<ud>uber_secret_channel</ud>" in tagged
-        assert "<ud>foo</ud>" in tagged           # everything is lower cased
+        assert "<ud>foo</ud>" in tagged  # everything is lower cased
         assert "<ud>bucket-user</ud>" in tagged
 
-        assert "<ud>baz</ud>" not in tagged       # passwords shouldn't be tagged, they get removed
+        assert (
+            "<ud>baz</ud>" not in tagged
+        )  # passwords shouldn't be tagged, they get removed
         assert "<ud>bucket-1</ud>" not in tagged  # bucket name is actually metadata
 
 
 class TestConvertToValidJSON(unittest.TestCase):
-
-    def basic_test(self):
-
+    def test_basic(self):
         invalid_json = r"""
         {
           "log": ["*"],
@@ -196,7 +195,7 @@ def basic_test(self):
         }
         """
 
-        valid_json = convert_to_valid_json(invalid_json)
+        valid_json = password_remover.convert_to_valid_json(invalid_json)
 
         got_exception = True
         try:
@@ -208,8 +207,7 @@ def basic_test(self):
 
         assert got_exception is False, "Failed to convert to valid JSON"
 
-    def basic_test_two_sync_functions(self):
-
+    def test_basic_two_sync_functions(self):
         invalid_json = r"""
             {
               "log": ["*"],
@@ -257,12 +255,12 @@ def basic_test_two_sync_functions(self):
                 }
                   }
                 `
-                },
+                }
               }
             }
             """
 
-        valid_json = convert_to_valid_json(invalid_json)
+        valid_json = password_remover.convert_to_valid_json(invalid_json)
 
         got_exception = True
         try:
@@ -276,7 +274,6 @@ def basic_test_two_sync_functions(self):
 
 
 class TestLowerKeys(unittest.TestCase):
-
     def test_basic(self):
         json_text_input = """{
            "Name": "Couchbase, Inc.",
@@ -293,7 +290,8 @@ def test_basic(self):
            ]
         }"""
         json_dict_actual = password_remover.lower_keys_dict(json_text_input)
-        json_dict_expected = json.loads("""{
+        json_dict_expected = json.loads(
+            """{
            "name": "Couchbase, Inc.",
            "address": {
               "street": "3250 Olcott St",
@@ -306,43 +304,48 @@ def test_basic(self):
               "Sync Gateway",
               "Couchbase Lite"
            ]
-        }""")
+        }"""
+        )
 
         # Sort the lists(if any) in both dictionaries before dumping to a string.
         json_text_actual = json.dumps(json_dict_actual, sort_keys=True)
         json_text_expected = json.dumps(json_dict_expected, sort_keys=True)
         assert json_text_expected == json_text_actual
 
-@pytest.mark.parametrize("input_str, expected", [
-       (
-			b'{"foo": "bar"}',
-			'{"foo": "bar"}',
-		),
-		(
-			b'{\"foo\": `bar`}',
-			'{"foo": "bar"}',
-		),
-		(
-			b'{\"foo\": `bar\nbaz\nboo`}',
-			r'{"foo": "bar\nbaz\nboo"}',
-		),
-		(
-			b'{\"foo\": `bar\n\"baz\n\tboo`}',
-			r'{"foo": "bar\n\"baz\n\tboo"}',
-		),
-		(
-			b'{\"foo\": `bar\n`, \"baz\": `howdy`}',
-			r'{"foo": "bar\n", "baz": "howdy"}',
-		),
-		(
-			b'{\"foo\": `bar\r\n`, \"baz\": `\r\nhowdy`}',
-			r'{"foo": "bar\n", "baz": "\nhowdy"}',
-		),
-		(
-			b'{\"foo\": `bar\\baz`, \"something\": `else\\is\\here`}',
-			r'{"foo": "bar\\baz", "something": "else\\is\\here"}',
-		),
-        ])
+
+@pytest.mark.parametrize(
+    "input_str, expected",
+    [
+        (
+            b'{"foo": "bar"}',
+            '{"foo": "bar"}',
+        ),
+        (
+            b'{"foo": `bar`}',
+            '{"foo": "bar"}',
+        ),
+        (
+            b'{"foo": `bar\nbaz\nboo`}',
+            r'{"foo": "bar\nbaz\nboo"}',
+        ),
+        (
+            b'{"foo": `bar\n"baz\n\tboo`}',
+            r'{"foo": "bar\n\"baz\n\tboo"}',
+        ),
+        (
+            b'{"foo": `bar\n`, "baz": `howdy`}',
+            r'{"foo": "bar\n", "baz": "howdy"}',
+        ),
+        (
+            b'{"foo": `bar\r\n`, "baz": `\r\nhowdy`}',
+            r'{"foo": "bar\n", "baz": "\nhowdy"}',
+        ),
+        (
+            b'{"foo": `bar\\baz`, "something": `else\\is\\here`}',
+            r'{"foo": "bar\\baz", "something": "else\\is\\here"}',
+        ),
+    ],
+)
 def test_convert_to_valid_json(input_str, expected):
     assert password_remover.convert_to_valid_json(input_str) == expected
     password_remover.get_parsed_json(input_str)