pgj commented on code in PR #4810: URL: https://github.com/apache/couchdb/pull/4810#discussion_r1374883989
########## src/docs/src/api/database/find.rst: ########## @@ -754,8 +760,10 @@ can itself be another operator with arguments of its own. This enables us to build up more complex selector expressions. However, only equality operators such as ``$eq``, ``$gt``, ``$gte``, ``$lt``, -and ``$lte`` (but not ``$ne``) can be used as the basis of a query. You should -include at least one of these in a selector. +``$lte`` and ``$beginsWith`` (but not ``$ne``) can be used as the basis Review Comment: The [serial comma](https://www.grammarly.com/blog/what-is-the-oxford-comma-and-why-do-people-care-so-much-about-it/) is missing before "and". By the way, is `$beginsWith` an equality operator? Others look about right since they are the standard comparison operators, but the prefix match is not like that. It might be the case that the original text is actually making a mistake here because, in the mathematical sense, equality has more to do with congruence and isomorphism than inequalities such as "less than" or "greater than". Comparisons like that have more requirements (an ordering) on the underlying structure compared to just telling if something is equal to something else or not. Sorry for the detour -- perhaps this all could be solved by saying that these are not equality but comparison operators? ########## src/mango/test/03-operator-test.py: ########## @@ -141,6 +142,38 @@ def test_exists_false_returns_missing_but_not_null(self): for d in docs: self.assertNotIn("twitter", d) + def test_beginswith(self): + self.db.save_docs( + [ + {"user_id": 99, "location": {"state": ":Bar"}}, + ] + ) + + cases = [ + {"prefix": "New", "user_ids": [2, 10]}, + # test characters that require escaping + {"prefix": "New ", "user_ids": [2, 10]}, + {"prefix": ":", "user_ids": [99]}, + {"prefix": "Foo", "user_ids": []}, + {"prefix": '"Foo', "user_ids": []}, + {"prefix": " New", "user_ids": []}, + ] + + for case in cases: + with self.subTest(prefix=case["prefix"]): + selector = {"location.state": {"$beginsWith": case["prefix"]}} + docs = self.db.find(selector) + self.assertEqual(len(docs), len(case["user_ids"])) + self.assertUserIds(case["user_ids"], docs) + + # non-string prefixes should return an error + def test_beginswith_invalid_prefix(self): + cases = [123, True, [], {}] + for prefix in cases: + with self.subTest(prefix=prefix): + with self.assertRaises(HTTPError): Review Comment: As far as I know, there is a different pattern in use throughout the test suite for testing cases like this one. Advantages: - There is no need to import `requests`, all the supporting code remains hidden in `mango`. - The actual HTTP status code could be asserted. Hence my recommendation: ```python for prefix in cases: with self.subTest(prefix=prefix): try: self.db.find({"location.state": {"$beginsWith": prefix}}) except Exception as e: self.assertEqual(e.response.status_code, 400) else: raise AssertionError("expected find error") ``` Curiously, you also use the same approach later in the change. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
