Skip to content
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

Use recommended Redis separator (colon ":") #192

Closed
Tracked by #290
aabrodskiy opened this issue Jul 4, 2022 · 5 comments · Fixed by #269
Closed
Tracked by #290

Use recommended Redis separator (colon ":") #192

aabrodskiy opened this issue Jul 4, 2022 · 5 comments · Fixed by #269
Assignees
Labels
enhancement New feature or request feature New functionality. good first issue Good for newcomers help wanted Extra attention is needed

Comments

@aabrodskiy
Copy link

aabrodskiy commented Jul 4, 2022

Thank you for maintaining this product!
It would be very convenient to use recommended and generally adopted Redis separator (colon) instead of the current "%&" separator.
This would make most Redis visualization tools automatically group items in the table and thus it would make debugging whole lot easier on large data sets.

This would be a small change in the return parameter of the below function:

def __get_primary_key(cls, primary_key_value: Any):
    """
    Returns the primary key value concatenated to the table name for uniqueness
    """
    table_name = (
        cls.__name__.lower() if cls._table_name is None else cls._table_name
    )
    return f"{table_name}_%&_{primary_key_value}"

return f"{table_name}:{primary_key_value}"

Screenshot 2022-07-04 at 18 51 09

"
@andrewthetechie
Copy link
Owner

Thanks for the feedback! This sounds like a valuable change.

I'd happily accept a PR making this change. I'll see if I get some free time to do it myself in the future

@andrewthetechie andrewthetechie added enhancement New feature or request good first issue Good for newcomers feature New functionality. help wanted Extra attention is needed labels Jul 10, 2022
@aabrodskiy
Copy link
Author

Sorry Andrew, I'd be happy to contribute but not an expert on how to do so on github unfortunately :(

@andrewthetechie
Copy link
Owner

andrewthetechie commented Jul 20, 2022

No problem.

If want to give it a try, check out https://docs.github.com/en/get-started/quickstart/contributing-to-projects as a guide to get started on forking and PRing on Github. Here is pydantic-aioredis's contributing guide https://github.com/andrewthetechie/pydantic-aioredis/blob/main/CONTRIBUTING.rst which has some documentation that might be helpful too

The tl;dr on the process would be:

  • fork this repo
  • Make the changes to your fork and commit them. pydantic-aioredis uses Conventional Commits for commit message https://www.conventionalcommits.org/en/v1.0.0/
  • Update tests and any documentation
  • Make a PR back to this repo

andrewthetechie added a commit that referenced this issue Sep 16, 2022
#192
#277
This adds the ability to customize the redis separator and changes the
default separator from _%&_ to :.

Additionally, it adds a way for a user to add a prefix to the key space
we use, to further differentiate their keys.

Finally, it fixes a bug with testing where redislite would not shutdown
properly and updates the fixtures to work with the newest version of
pytest_asyncio.

BREAKING CHANGE: This will result in "data loss" for existing models
stored in redis due to the change in default separator. To maintain
backwards compatbility with 0.7.0 and below, you will need to modify
your existing models to set _redis_separator = "_%&_" as a field on
them.
andrewthetechie added a commit that referenced this issue Sep 16, 2022
#192
#277
This adds the ability to customize the redis separator and changes the
default separator from _%&_ to :.

Additionally, it adds a way for a user to add a prefix to the key space
we use, to further differentiate their keys.

Finally, it fixes a bug with testing where redislite would not shutdown
properly and updates the fixtures to work with the newest version of
pytest_asyncio.

BREAKING CHANGE: This will result in "data loss" for existing models
stored in redis due to the change in default separator. To maintain
backwards compatbility with 0.7.0 and below, you will need to modify
your existing models to set _redis_separator = "_%&_" as a field on
them.
andrewthetechie added a commit that referenced this issue Sep 17, 2022
* feat: redis-separator

#192
#277
This adds the ability to customize the redis separator and changes the
default separator from _%&_ to :.

Additionally, it adds a way for a user to add a prefix to the key space
we use, to further differentiate their keys.

Finally, it fixes a bug with testing where redislite would not shutdown
properly and updates the fixtures to work with the newest version of
pytest_asyncio.

BREAKING CHANGE: This will result in "data loss" for existing models
stored in redis due to the change in default separator. To maintain
backwards compatbility with 0.7.0 and below, you will need to modify
your existing models to set _redis_separator = "_%&_" as a field on
them.

* ci: update tests to test new features

* ci: update noxfile and requirements for testing

* fix: fix for py3.7 and 3.8

* fix: main branch poetry.lock and constraints

* fix: fix import order
@andrewthetechie
Copy link
Owner

andrewthetechie commented Sep 17, 2022

#269

This feature will be included in the 1.0.0 release once that PR is merged. I'm shooting to have it merged in the next 7 days.

@andrewthetechie andrewthetechie self-assigned this Sep 17, 2022
@andrewthetechie andrewthetechie linked a pull request Sep 17, 2022 that will close this issue
@andrewthetechie
Copy link
Owner

This feature is included in https://github.com/andrewthetechie/pydantic-aioredis/releases/tag/v1.0.0 which is available in pypi now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request feature New functionality. good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants