-
Notifications
You must be signed in to change notification settings - Fork 92
Deprecate the ~log table.. Improve code organization: docstrings, type hints, and imports. #1297
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
base: claude/refine-primary-key-rules-lVgTt
Are you sure you want to change the base?
Conversation
- Add module docstrings to cli.py, admin.py, logging.py, and hash.py - Add `from __future__ import annotations` for PEP 563 deferred evaluation - Modernize type hints using Python 3.10+ syntax (X | None instead of Optional[X]) - Use TYPE_CHECKING imports to avoid circular dependencies - Improve function docstrings to Google style with Args/Returns sections - Add type annotations to key public APIs in: - connection.py: Connection class and conn() function - fetch.py: Fetch and Fetch1 classes - expression.py: QueryExpression class properties and methods - utils.py: utility functions - hash.py: hashing functions - admin.py: kill() and kill_quick() functions
- Add module docstrings to cli.py, admin.py, logging.py, and hash.py - Add `from __future__ import annotations` for PEP 563 deferred evaluation - Modernize type hints using Python 3.10+ syntax (X | None instead of Optional[X]) - Use TYPE_CHECKING imports to avoid circular dependencies - Improve function docstrings to Google style with Args/Returns sections - Add type annotations to: - admin.py: set_password(), kill(), and kill_quick() functions - cli.py: cli() function - hash.py: all hashing functions - logging.py: excepthook() function - utils.py: all utility functions and ClassProperty Co-authored-by: dimitri-yatsenko <[email protected]>
…ew-code-organization-9135z Merges restructured project with src/datajoint/ layout and code organization improvements including docstrings, type hints, and modern Python 3.10+ features. Co-authored-by: dimitri-yatsenko <[email protected]>
90a1b71 to
4eb8202
Compare
Closes #1298 The ~log table is a client-side audit logging mechanism that has been superseded by modern alternatives including: - Server-side database audit logging - Infrastructure logging platforms (Datadog, CloudWatch) - Python's standard logging module Changes: - Remove Log class from table.py - Remove log property from Schema class - Replace self._log() calls with logger.info/debug - Remove special ~log handling from heading.py - Delete test_log.py Existing ~log tables in databases remain but will no longer be written to. All operations now emit through Python's logging module instead. Co-authored-by: dimitri-yatsenko <[email protected]>
The package was moved to src/datajoint/ but these files remained as orphans after the merge. Removing to avoid confusion. Co-authored-by: dimitri-yatsenko <[email protected]>
| self.f = f | ||
|
|
||
| def __get__(self, obj, owner): | ||
| def __get__(self, obj: Any, owner: type) -> Any: |
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.
object is generally a better type annotation than Any when working with unknown python objects. An annotation of object basically says "this could be any python object", whereas the annotation of Any says "don't do any type-checking against this value".
and why does this function take obj, despite not doing anything with it?
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.
ClassProperty was introduced probably around Python 3.4 days and I wonder if it can be replaced altogether. Looking into it...
|
|
||
| def safe_copy(src, dest, overwrite=False): | ||
| def safe_copy( | ||
| src: str | 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.
if str | Path is used a lot in the codebase, you might want to introduce a type alias in one place like PathLike = str | Path. This adds indirection however, which is a drawback.
| *, | ||
| init_fun: str | None = None, | ||
| reset: bool = False, | ||
| use_tls: bool | dict[str, Any] | None = None, |
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.
dict[str, object] should be preferred over dict[str, Any]
| return self | ||
|
|
||
| def __next__(self): | ||
| def __next__(self) -> tuple | dict: |
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.
tuple and dict need type parameters
| return next(self._iter) | ||
|
|
||
| def fetchall(self): | ||
| def fetchall(self) -> list[tuple | dict]: |
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.
tuple and dict need type parameters
| return self._data | ||
|
|
||
| def fetchone(self): | ||
| def fetchone(self) -> tuple | dict: |
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.
tuple and dict need type parameters
| self._conn.close() | ||
|
|
||
| def register(self, schema): | ||
| def register(self, schema: Any) -> None: |
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.
is schema really Any?
|
|
||
|
|
||
| def key_hash(mapping): | ||
| def key_hash(mapping: Mapping) -> str: |
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.
mapping needs type parameters
|
if you want a fancier logging solution, I like |
d-v-b
left a comment
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.
looks good, just some type annotations that require parameters (dict, list, Mapping) and a request to use object instead of Any as a catch-all type
Fix #1298 - Deprecate the
~logtable.from __future__ import annotationsfor PEP 563 deferred evaluation