fixed problems in #143

[amirreza8002]

closes #143

[aiven-sal]

Thanks! Could you also add test cases?

[mkmkme]

Could you please add tests for both improvements you introduced?

[amirreza8002]

i pushed the last commit to be discussed here

i think since the type hint is explicit that no string should be passed, having those two lines is a bit weird
let me know what you think

[mkmkme]

@aiven-sal we discussed with @amirreza8002 in matrix channel the change in c4de8a5. The current typing of the function does not allow passing a string as ex, so passing it at least will trigger the mypy warning. So this extra case looks really off, and it's quite inconsistent with the rest of the arguments. I agreed to remove it.

Is it okay with you?

[mkmkme]

@amirreza8002 the linting has failed. Could you fix it please?

EDIT: also the tests have failed

[mkmkme]

So it seems that at least there were some users of ex being string :) So maybe we should not remove that handling...

[amirreza8002]

the test fail is for the last commit, I'll adjust the test if the change is approved,

sorry about the lint, I'm used to my ide doing it :/
I'll fix that up

[aiven-sal]

@aiven-sal we discussed with @amirreza8002 in matrix channel the change in c4de8a5. The current typing of the function does not allow passing a string as ex, so passing it at least will trigger the mypy warning. So this extra case looks really off, and it's quite inconsistent with the rest of the arguments. I agreed to remove it.

Is it okay with you?

My impression (which might be not 100% right) is that many valkey-py functions accept ints and numerical strings interchangeably. This may be not always intentional, but disallowing strings now is a breaking change. I tend to be against breaking changes, but if we do it in a major release I won't oppose it.

[mkmkme]

So @amirreza8002 would you please revert the last commit instead of fixing the test?

We really don't want to break anything right now

[amirreza8002]

My impression (which might be not 100% right) is that many valkey-py functions accept ints and numerical strings interchangeably. This may be not always intentional, but disallowing strings now is a breaking change. I tend to be against breaking changes, but if we do it in a major release I won't oppose it.

so do you think, for a major version, other parameters should alow strings, or should we remove the string support?
e.g. right now onle ex allows string, px, exat and pxat don't

[mkmkme]

so do you think, for a major version, other parameters should alow strings, or should we remove the string support?
e.g. right now onle ex allows string, px, exat and pxat don't

I'd personally leave the arguments as they are currently. And we can put a mental note for the next major release to remove string support for ex

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.22%. Comparing base (013fef0) to head (fbb232e).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #144      +/-   ##
==========================================
+ Coverage   76.21%   76.22%   +0.01%     
==========================================
  Files         130      130              
  Lines       33923    33940      +17     
==========================================
+ Hits        25854    25871      +17     
  Misses       8069     8069              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[mkmkme]

Hey @amirreza8002 ! Do I understand correctly that the changes are ready for review and to be merged?

If so, would you rebase your PR on the top of the current main branch?

[amirreza8002]

hi
yes, afaik it can be merged

[amirreza8002]

hi
is this a problem from my side?
the test's error doesn't seem relevant to my code
I'm not sure what the other fail is for

[aiven-sal]

hi is this a problem from my side? the test's error doesn't seem relevant to my code I'm not sure what the other fail is for

Hi.
The problem is that you merged instead of rebasing.
We don't want merge commits in PRs (they make the git history difficult to look back at).
To fix the problem you should rebase and remove the merge commit.

[amirreza8002]

i hope that fixed it?
the problem was that i thought using github's sync would rebase, it seems like it doesn't :)

[mkmkme]

I think it should be fixed now, yes. I've approved the github actions.

the problem was that i thought using github's sync would rebase, it seems like it doesn't

AFAIK you can choose the way you want to update the branch.

[amirreza8002]

i have some follow up for this, but i think it should be done in another pr

[mkmkme]

i have some follow up for this, but i think it should be done in another pr

What do you have in mind?

[amirreza8002]

i have three things in mind

1. if valkey accepts strings, why cast it to int in ex

2. for px, exat and pxat it might be appropriate to add check to see if the string is digit, and not hi

3. if string is supported might as well add it to type hints

[mkmkme]

if valkey accepts strings, why cast it to int in ex

I'm not a Valkey expert, but my understanding is that it only supports the string representations of numbers. But I'll have to double-check that.

In general, your questions are valid, but I'd hesitate to change any user experience at this moment.

[mkmkme]

The changes now look good to me.
Thanks! And sorry it was a bit painful to land :)

[amirreza8002]

but I'd hesitate to change any user experience at this moment.

I'm not sure how this effects user xp, but i respect the packages development plans

I was just thinking ahead, i don't think I'll be working on it any time soon, probably do some type hints for the time being since I'm doing similar work in django-valkey