+
Skip to content
This repository was archived by the owner on Aug 23, 2022. It is now read-only.

Conversation

vikahl
Copy link
Contributor

@vikahl vikahl commented Jul 8, 2021

Improve the data store and update SQLAlchemy usage to 2.0 syntax.

Adds types to the function definition, improve the data store tests and general
code quality.

Test plan

  • Ensure CI checks passes.
  • Get someone to review the code to spot obvious errors.
  • Run the branch in a complete Comet implementation.

When the above has been done the PR can be merged and released.

Potential breaking changes

Removal of add_event

This PR removes the unused function "add_event" from the data store. This could
potentially impact existing usage, but it is not recommended to interact with
the "private" functions directly.

Change of models repr

The models unique string representation (__repr__) has been updated to only
include relevant information, to improve readability.

All changes

Improve tests, especially around data store (aed1a5d)

Re-define test utils as pytest fixtures for easier usage.

Generalise pytest fixtures used for the data store and chain the
fixtures so that the initialised data store-fixture depends on the data
store-fixture etc. This will make it easier to do test-related changes
in the objects.

The fixtures can be improved further, there seems to be no clear
consensus if the tests should create/load it's own data or if they
should use the pre-populated fixtures. The current state is working, but
the tests would be easier to read if they relied more on fixtures.

Add docstrings to the data store tests, explaining what the tests do.

Clean up some test code by removing duplicate tests, splitting some
tests that do many things and remove unnecessary assert statements
(e.g., first checking if the object is not None, then if it is a
datetime instance. Only the datetime instance check is needed as it will
then not be None).

Add types, update repr and improve docs for data_store and models (70b4cc9)

Add types for data_store and model, as part of an effort to gradually
roll out types in the code base. Related to #56.

Change import structure for sqlalchemy and only import the top level
package, to make the code easier to read and understand where e.g.,
"Column" comes from.

Improve the docstrings by attempting to have a first line summary and
additional description in follow lines. Types were removed from the
docstring as they are part of the function signature.
The docstring is written in 120 character lines, given that the codebase
is using them, but it would be easier to read in terminal usage if they
were 80 char lines.

Update the repr method of models to print only the relevant files
(and remove a bunch of unnecessary code).

Add mypy config to gradually roll out types (4a560cb)

Add mypy config to gradually roll out types in the code base. The
configured tox environment will not run unless explicitly specified
(with tox -e types).

The mypy config will ignore all files that have not been opted in,
allowing a gradual roll out through the codebase.

Start refactoring SQLAlchemy usage for 1.4/2.0 (b5d157a)

Start refactoring the code in the datastore to follow SQLAlchemy 2.0
syntax.
Something is up with equality checking, hence repr in test

Co-authored-by: Mattias Springare mspringare@gmail.com

@vikahl vikahl force-pushed the refactor-sql-alchemy-usage branch from 571f3b2 to 4ffa6e3 Compare July 15, 2021 07:42
@vikahl vikahl force-pushed the refactor-sql-alchemy-usage branch 5 times, most recently from 62634e3 to 5f3cc1b Compare August 5, 2021 12:13
@vikahl vikahl changed the title Start refactoring SQLAlchemy usage for 1.4/2.0 Improve data store Aug 5, 2021
@vikahl vikahl marked this pull request as ready for review August 5, 2021 12:34
@vikahl vikahl force-pushed the refactor-sql-alchemy-usage branch from 5f3cc1b to 6fb1d54 Compare August 5, 2021 12:35
vikahl and others added 5 commits August 17, 2021 11:28
Start refactoring the code in the datastore to follow SQLAlchemy 2.0
syntax.
Something is up with equality checking, hence __repr__ in test

Co-authored-by: Mattias Springare <mspringare@gmail.com>
Add mypy config to gradually roll out types in the code base. The
configured tox environment will not run unless explicitly specified
(with tox -e types).

The mypy config will ignore all files that have not been opted in,
allowing a gradual roll out through the codebase.
Add types for data_store and model, as part of an effort to gradually
roll out types in the code base. Related to #56.

Change import structure for sqlalchemy and only import the top level
package, to make the code easier to read and understand where e.g.,
"Column" comes from.

Improve the docstrings by attempting to have a first line summary and
additional description in follow lines. Types were removed from the
docstring as they are part of the function signature.
The docstring is written in 120 character lines, given that the codebase
is using them, but it would be easier to read in terminal usage if they
were 80 char lines.

Update the __repr__ method of models to print only the relevant files
(and remove a bunch of unnecessary code).
Re-define test utils as pytest fixtures for easier usage.

Generalise pytest fixtures used for the data store and chain the
fixtures so that the initialised data store-fixture depends on the data
store-fixture etc. This will make it easier to do test-related changes
in the objects.

The fixtures can be improved further, there seems to be no clear
consensus if the tests should create/load it's own data or if they
should use the pre-populated fixtures. The current state is working, but
the tests would be easier to read if they relied more on fixtures.

Add docstrings to the data store tests, explaining what the tests do.

Clean up some test code by removing duplicate tests, splitting some
tests that do many things and remove unnecessary assert statements
(e.g., first checking if the object is not None, then if it is a
datetime instance. Only the datetime instance check is needed as it will
then not be None).
@vikahl vikahl force-pushed the refactor-sql-alchemy-usage branch 2 times, most recently from 7cc744d to 93e3af9 Compare August 18, 2021 08:55
@bgres bgres merged commit 6610cd4 into master Aug 27, 2021
@bgres bgres deleted the refactor-sql-alchemy-usage branch August 27, 2021 07:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

点击 这是indexloc提供的php浏览器服务,不要输入任何密码和下载