chore: static analysis hack for globals

[ankush]

Since frappe.db is dynamically defined static code analyzers can't understand what it is. typing.TYPE_CHECKING can be used to instructs static code analyzers what to do with such globals.

Reference: https://docs.python.org/3/library/typing.html#typing.TYPE_CHECKING

Outcome: Depending on your editor/config now frappe.db.* should auto-complete, jump to the definition and give docstrings on hover etc.

Note:

• this is NOT py2 compatible.
• Comment at end of if-block is intentional. This is to avoid accidentally leaking some code outside of if block.
• I've only added frappe.db for now, more can be added if required.

Screen.Recording.2021-04-09.at.9.35.01.PM.mov

[Alchez]

@ankush, shouldn't this detect the active db type (mariadb or postgres), and import the correct controller accordingly?

[ankush]

@Alchez I do not think it's possible to detect it, as it comes from config files and static analyzers won't actually run the code to find the right database type. Alternate is to assign db to base class Database.

ref:

frappe/frappe/__init__.py

Line 214 in 638a580

local.db = get_db(user=db_name or local.conf.db_name)

frappe/frappe/__init__.py

Line 174 in 638a580

local.conf = _dict(get_site_config())

[Alchez]

static analyzers won't actually run the code to find the right database type

Static analyzers don't execute TYPE_CHECKING code at runtime, it's true, but I think they do get executed during analysis. So they would be able to retrieve info from config files.

Alternate is to assign db to base class Database.

This is also a way, but you'll miss out on the other controller methods that may not exist in Database.

[ankush]

Pyright does not evaluate basic string comparison:

VS code with Jedi evaluates basic string comparison but doesn't evaluate config:

[ankush]

@Alchez see updated commit. Thanks for pointing it out, I think from type analysis perspective it is better to define type as Union of both. So code can be checked for compatibility against both types.

In editors, the behaviour of this is kinda dependent on implementation of the editor, some would just jump to first/common definition and some will ask user for picking between two implementations. Anyway, I feel this is better than hardcoding mariadb as default db type.

[Alchez]

@ankush Agreed, union should be good enough. The dev can then go in and see all references.

[kisg]

Why don't you just use a standard type annotation like this:

from frappe.database import Database
db : Database = local("db")

I think that it is a dangerous pattern for application code to call methods defined in either MariaDBDatabase or PostgresDatabase on frappe.db instead of the real interface defined in Database. If database specific functionality is required, then that should be done like this:

T = TypeVar('T', bound='Database')

class Database:
    def get_impl(self: T) -> T:
        return self


frappe.db.get_impl().specificmethod() # Use DB specific method

Check the docs here: https://mypy.readthedocs.io/en/stable/generics.html#generic-methods-and-generic-self

A potential problem could be with the used local("db") initialization, but I think that could be solved with some refactoring.

[ankush]

Thanks, @kisg for your suggestion.

from frappe.database import Database
db : Database = local("db")

We do not want to import Database in frappe. frappe module gets imported in many places it's an unnecessary import cost. (especially in CLI)

As of now, there are only two databases officially supported in frappe and only one in erpnext. So technically Union[MariaDB, PostgresDB] is the same as Database as union is supposed to force check for implementation with both classes. Correct me if I am wrong here.

Also at present we are not type checking at all, I am working on adding types to some of the core functionality gradually. Only after that, the second part of your suggestion would apply.

A potential problem could be with the used local("db") initialization, but I think that could be solved with some refactoring.

Open for suggestions 😄

[kisg]

Thanks, @kisg for your suggestion.

from frappe.database import Database
db : Database = local("db")

We do not want to import Database in frappe. frappe module gets imported in many places it's an unnecessary import cost. (especially in CLI)

The Database will be imported when you call get_db() anyways, so I am not sure if there is anything gained here. Do you have any benchmarks for this?
However, an even better solution could be to define an abstract IDatabase interface class (with proper static type annotations), and import only that.

As of now, there are only two databases officially supported in frappe and only one in erpnext. So technically Union[MariaDB, PostgresDB] is the same as Database as union is supposed to force check for implementation with both classes. Correct me if I am wrong here.

I am hoping to add support for PostgreSQL in ERPNext, at least I already started looking at it.

My concern is that you are adding code that introduces a dependency on the exact DB types instead of decoupling it.

Also: Union[A, B] means that either A or B type is allowed, which is not the same as using the supertype of A and B. The Union[] version allows any custom API defined in either A or B, in our case any MariaDB or Postgres specific function can be used anywhere and the type checker will be perfectly fine with it. But it will break at runtime.

The purpose of static type checking should be to catch these kind of errors, in my opinion.

Also at present we are not type checking at all, I am working on adding types to some of the core functionality gradually. Only after that, the second part of your suggestion would apply.

A potential problem could be with the used local("db") initialization, but I think that could be solved with some refactoring.

Open for suggestions 😄

Let's agree on the above points first. :)

[ankush]

Also: Union[A, B] means that either A or B type is allowed, which is not the same as using the supertype of A and B. The Union[] version allows any custom API defined in either A or B, in our case any MariaDB or Postgres specific function can be used anywhere and the type checker will be perfectly fine with it. But it will break at runtime.

Not true.

Ref: https://mypy.readthedocs.io/en/stable/kinds_of_types.html#union-types

Operations are valid for union types only if they are valid for every union item.

Try this toy example:

import typing
from typing import Union


class MariaDB():
	def mariadb_method(self):
		pass

	def common(self):
		pass


class Postgres():
	def postgres_method(self):
		pass

	def common(self):
		pass


db: Union[MariaDB, Postgres] = MariaDB()

db.mariadb_method()
db.postgres_method()
db.common()

mypy output:

test.py:23: error: Item "Postgres" of "Union[MariaDB, Postgres]" has no attribute "mariadb_method"
test.py:24: error: Item "MariaDB" of "Union[MariaDB, Postgres]" has no attribute "postgres_method"
Found 2 errors in 1 file (checked 1 source file)

[kisg]

Also: Union[A, B] means that either A or B type is allowed, which is not the same as using the supertype of A and B. The Union[] version allows any custom API defined in either A or B, in our case any MariaDB or Postgres specific function can be used anywhere and the type checker will be perfectly fine with it. But it will break at runtime.

Not true.

Ref: https://mypy.readthedocs.io/en/stable/kinds_of_types.html#union-types

You are right, I should have double checked in mypy (I only checked in PEP-484, where this is not explicit). Sorry for the confusion.

Still, the fact that both MariaDB and Postgres implementation classes have the same attribute does not mean that they are actually meant to be used by a client directly in DB independent code. I think it is a cleaner approach to define the DB interface explicitly, and using that in static typing.

[netchampfaris]

@kisg I wouldn't say Frappe Framework is fully DB independent yet. Even the current Postgres support is not fully functional in some actions like Rename.

But this change improves the DX right now for all developers working on Frappe. So, until we have better support for multiple databases, we can merge this.