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

fix issue #656: add 'modules' keyword arg to load Redis extension modules #661

Merged
merged 9 commits into from
Jun 5, 2024

Conversation

mguijarr
Copy link

@mguijarr mguijarr commented May 10, 2024

Here is a pull request to close #656 , taking into account comments in : #657

(sorry had to create a new pull request... Not super at ease with github vs gitlab !)

newsfragments/656.misc.rst Outdated Show resolved Hide resolved
@fizyk
Copy link
Member

fizyk commented May 10, 2024

@mguijarr one last thing.... would you be able to add some tests as well?

@fizyk
Copy link
Member

fizyk commented May 10, 2024

Oh, one last thing, please mention in the documentation that it will not work for noproc fixtures.

@mguijarr
Copy link
Author

@mguijarr one last thing.... would you be able to add some tests as well?

I added a test, just for modules keyword argument (didn't find the tests for pytest.ini and pytest command line args)

@mguijarr
Copy link
Author

Oh, one last thing, please mention in the documentation that it will not work for noproc fixtures.

I thought it was done already ? In the table, it has - for noproc-fixture column

@fizyk
Copy link
Member

fizyk commented May 13, 2024

I thought it was done already ? In the table, it has - for noproc-fixture column

Yeah, the column lists configuration for the fixture if you'd want to use the fixture factory, and pass your own values there. There's nothing else stating that if you configure from command line or config file, it will not affect the redis connected to through noproc fixture.

tests/test_executor.py Outdated Show resolved Hide resolved
@mguijarr
Copy link
Author

I thought it was done already ? In the table, it has - for noproc-fixture column

Yeah, the column lists configuration for the fixture if you'd want to use the fixture factory, and pass your own values there. There's nothing else stating that if you configure from command line or config file, it will not affect the redis connected to through noproc fixture.

I just added more descriptive documentation

@fizyk
Copy link
Member

fizyk commented May 17, 2024

@mguijarr now, there are some issues with mypy now and the ruff linter left :)

@mguijarr
Copy link
Author

Sorry for the long delay, have been busy - ok I tried to make mypy happy and I applied ruff

@mguijarr
Copy link
Author

Ok thanks for approval to let tests pass.. Again it fails with mypy 😭

No idea what is wrong

@mguijarr
Copy link
Author

I don't use mypy maybe you can make the fix yourself ? If you don't mind

@fizyk
Copy link
Member

fizyk commented May 29, 2024

I'll try to take a look sometimes next week :)

@mguijarr
Copy link
Author

mguijarr commented Jun 4, 2024

Looks nice 🥳 Is it ready to merge soon?

@fizyk fizyk merged commit 58fc12e into ClearcodeHQ:main Jun 5, 2024
31 checks passed
@mguijarr
Copy link
Author

mguijarr commented Jun 5, 2024

Thanks a lot! 👍🏻 And a great thank you for pytest-redis, very useful

@fizyk
Copy link
Member

fizyk commented Jun 5, 2024

Thank you for your contribution :)

Wait a bit, till I add python 3.12 proper support, and redis 7.2 into tests and I'll be releasing new version :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

no way to pass --loadmodule argument to command line
2 participants