Implement mypy

[Ce11an]

Thank you for submitting this pull request! We appreciate you spending the time to work on these changes.

What is the motivation?

Adding a static type checker. mypy makes it easier to find bugs with less debugging.

Type of Change

• 💥 Breaking change (fix or feature that would cause existing functionality to change)

What does this change do?

• Adds mypy as a development dependency.
• Corrects all errors found when running mypy.
• Made API changes if necessary.
• Fixed docstrings where necessary.
• Added mypy and black to GitHub Actions.

What is your testing strategy?

mypy runs with no errors. API documentation will be updated with any breaking changes. From discussing with @AlexFrid (#59), it was agreed that mypy errors can be ignored until the HTTP client has been refactored.

Is this related to any issues?

#39

Have you read the Contributing Guidelines?

• I have read the Contributing Guidelines

[Ce11an]

This PR is a draft until the GitHub action passes.

[Ce11an]

Most notable breaking change is here. Also, the connect method no longer takes arguments. Let me know your thoughts.

[PythonTryHard]

Potential duplicated code that you missed?

@@ -562,15 +540,15 @@ class Surreal:
             ),
         )
         _validate_response(response, SurrealPermissionException)
-        # success: ResponseSuccess = _validate_response(
-            # response, SurrealPermissionException
-        # )
-        # return success.result
+        success: ResponseSuccess = _validate_response(
+            response, SurrealPermissionException
+        )
+        return success.result

[PythonTryHard]

Looks good to me!

[AlexFrid]

This PR is a draft until the GitHub action passes.

I see that the PR is not a draft now. Does that mean that you have tested and confirmed the GitHub action works as expected?

[AlexFrid]

Most notable breaking change is here. Also, the connect method no longer takes arguments. Let me know your thoughts.

I think the way we connect and such will eventually need to be refactored 🤔

Would need some time tough to think and see what the implications of this change are 🤔

What is your reasoning for the change?

[Ce11an]

This PR is a draft until the GitHub action passes.

I see that the PR is not a draft now. Does that mean that you have tested and confirmed the GitHub action works as expected?

Yes, it is working. You can see the output here.

[Ce11an]

Most notable breaking change is here. Also, the connect method no longer takes arguments. Let me know your thoughts.

I think the way we connect and such will eventually need to be refactored 🤔

Would need some time tough to think and see what the implications of this change are 🤔

What is your reasoning for the change?

I have a couple of reasons (I am open to suggestions/feedback!):

• mypy was complaining that the a URL could be inputted into two different places. Either a URL should be passed in the __init__ method or connect method not both.
• The Surreal class controls interactions with a database. In my opinion, the class is easier to use (eventually test) if each instantiated Surreal class controls a respective database connection.
• I am opting for passing the url in the __init__ due to the examples below.

This example is confusing because you don't know what database db is connected to:

db = Surreal()

db.connect("url/1")
db.query("query_1")

db.connect("url/2")
db.query("query_2")

As opposed to

db_1 = Surreal("url/1")
db_2 = Surreal("url/2")

db_1.connect()
db_1.query("query_1")

db_2.connect()
db_2.query("query_2")

Let me know if I have missed anything! 👍🏻

[AlexFrid]

Most notable breaking change is here. Also, the connect method no longer takes arguments. Let me know your thoughts.

I think the way we connect and such will eventually need to be refactored 🤔
Would need some time tough to think and see what the implications of this change are 🤔
What is your reasoning for the change?

I have a couple of reasons (I am open to suggestions/feedback!):

• mypy was complaining that the a URL could be inputted into two different places. Either a URL should be passed in the __init__ method or connect method not both.
• The Surreal class controls interactions with a database. In my opinion, the class is easier to use (eventually test) if each instantiated Surreal class controls a respective database connection.
• I am opting for passing the url in the __init__ due to the examples below.

This example is confusing because you don't know what database db is connected to:

db = Surreal()

db.connect("url/1")
db.query("query_1")

db.connect("url/2")
db.query("query_2")

As opposed to

db_1 = Surreal("url/1")
db_2 = Surreal("url/2")

db_1.connect()
db_1.query("query_1")

db_2.connect()
db_2.query("query_2")

Let me know if I have missed anything! 👍🏻

We have talked about this internally, also how it will be with Rust going forward, and it seems the best is indeed to have the URL just go into the Surreal class, but also just get rid of db.connect() as a public method 🤔

db_1 = await Surreal("url/1")
db_2 = await Surreal("url/2")

db_1.query("query_1")
db_2.query("query_2")

[Ce11an]

Most notable breaking change is here. Also, the connect method no longer takes arguments. Let me know your thoughts.

I think the way we connect and such will eventually need to be refactored 🤔
Would need some time tough to think and see what the implications of this change are 🤔
What is your reasoning for the change?

I have a couple of reasons (I am open to suggestions/feedback!):

• mypy was complaining that the a URL could be inputted into two different places. Either a URL should be passed in the __init__ method or connect method not both.
• The Surreal class controls interactions with a database. In my opinion, the class is easier to use (eventually test) if each instantiated Surreal class controls a respective database connection.
• I am opting for passing the url in the __init__ due to the examples below.

This example is confusing because you don't know what database db is connected to:

db = Surreal()

db.connect("url/1")
db.query("query_1")

db.connect("url/2")
db.query("query_2")

As opposed to

db_1 = Surreal("url/1")
db_2 = Surreal("url/2")

db_1.connect()
db_1.query("query_1")

db_2.connect()
db_2.query("query_2")

Let me know if I have missed anything! 👍🏻

We have talked about this internally, also how it will be with Rust going forward, and it seems the best is indeed to have the URL just go into the Surreal class, but also just get rid of db.connect() as a public method 🤔

db_1 = await Surreal("url/1")
db_2 = await Surreal("url/2")

db_1.query("query_1")
db_2.query("query_2")

Ah, great. However, I think it would be important to keep the connect and disconnect methods public for developer flexibility. In my opinion, the Surreal class should not have connection login in the __init__. This could lead to potential side affects and is less flexible.

To give some examples of my proposal:

db = Surreal("url/1")
await db.connect()
await db.query("query_1")
await db.disconnect()

And

async with Surreal("url/1"):
    await db.query("query_1")

What do you think?

[Ce11an]

I have added extra mypy arguments to the pyproject.toml. If anyone thinks more or less should be added you can find the definitions here. Thanks @huenique 👍🏻

[huenique]

Hey @Ce11an, I'd like to suggest a few more changes to the ws.py module. I placed them in the following gist since I didn't want to litter the PR with comments: https://gist.github.com/huenique/a9b8f80404b90ae889963b6750089df6

I did some refactoring to make some parts of the code more robust. Please check out the [git] diff and see what you can use from the changes. Thanks!

[Ce11an]

Hey @Ce11an, I'd like to suggest a few more changes to the ws.py module. I placed them in the following gist since I didn't want to litter the PR with comments: https://gist.github.com/huenique/a9b8f80404b90ae889963b6750089df6

I did some refactoring to make some parts of the code more robust. Please check out the [git] diff and see what you can use from the changes. Thanks!

Great - Thanks 😄 Might be worth creating an issue or doing a separate PR for the proposed changes as I don't want to change too much in this PR 😅

[huenique]

Great - Thanks 😄 Might be worth creating an issue or doing a separate PR for the proposed changes as I don't want to change too much in this PR 😅

Understandable. Fixes to the type hints aren't really within scope for this PR, so it is more appropriate to make further corrections to the type annotations in a separate PR. Thanks!

[Ce11an]

@AlexFrid What do you think about the changes purposed? I am working on a PR for unit tests and they depend on whether this PR gets merged or not.

I've been learning Rust. It looks like the new method acts as a constructor in Rust. So having the URL in the __init__ would make sense. Let me know 👍🏼

[AlexFrid]

@AlexFrid What do you think about the changes purposed? I am working on a PR for unit tests and they depend on whether this PR gets merged or not.

I've been learning Rust. It looks like the new method acts as a constructor in Rust. So having the URL in the __init__ would make sense. Let me know 👍🏼

Yeah, I think the changes make sense.
I'm still thinking if there is a way to do the Python equivalent of the Rust
let db = Surreal::new::<Ws>("127.0.0.1:8000").await?;
Which would be db = await Surreal("127.0.0.1:8000") 🤔

But I could think of that in a different PR, I think the changes made here make sense and don't want to unnecessarily delay this progress, so I'll be merging this but waiting a bit before releasing a new version on PyPi, which will most likely happen end of this week or beginning of next as the docs will need updating as well.