Enhance code readability #241

nimanikoo · 2024-01-26T17:31:29Z

Hello, I hope you are well
Thank you for helping open source and building this project for the .NET community
Out of curiosity, I was reviewing the codes of this repository and I found some points that might be good to correct.
One of the performance improvements was where you used lists
The next thing is that you can use the primary constructor according to the latest version of .NET
In some cases, we could have written the code in a more concise and readable manner and used expressions
There were several typo problems that I corrected
I hope these things are helpful and I will be happy to contribute to the progress of this project
Please check and let me know the result

Fix some typos in the markdown files and run `dotnet format` on existing code.

Change the settings so that documentation is included in the build.

Add support for the BZMPOP command. This command is blocking on the server, so it goes against the current policy of the StackExchange.Redis library. Therefore make it obvious in the code documentation that attention must be given to the timeout in the connection multiplexer. The StackExchange.Redis library already defines a type for the payload returned by BZMPOP (which is the same as for ZMPOP), namely the SortedSetPopResult class. However, the constructor of that class is internal in the library, so we can't create instances of it. Therefore roll our out type for a <value, score> pair, and use Tuple to pair a key with a list of such <value, score> pairs.

…formance and memory usage

…ge foreach to linq expression

…sAsync.cs and JsonTests.cs

codecov-commenter · 2024-01-28T15:18:23Z

Codecov Report

Attention: Patch coverage is 93.76900% with 41 lines in your changes are missing coverage. Please review.

Project coverage is 94.35%. Comparing base (ccd74b5) to head (4159b23).

Files	Patch %	Lines
src/NRedisStack/Bloom/BloomCommandBuilder.cs	77.27%	5 Missing and 5 partials ⚠️
src/NRedisStack/Search/AggregationResult.cs	73.33%	6 Missing and 2 partials ⚠️
...rc/NRedisStack/CountMinSketch/CmsCommandBuilder.cs	84.21%	3 Missing and 3 partials ⚠️
src/NRedisStack/Search/DataTypes/InfoResult.cs	90.47%	1 Missing and 5 partials ⚠️
src/NRedisStack/Search/Document.cs	87.50%	3 Missing and 2 partials ⚠️
src/NRedisStack/CoreCommands/CoreCommands.cs	97.29%	1 Missing and 1 partial ⚠️
src/NRedisStack/Json/JsonCommandBuilder.cs	90.90%	0 Missing and 2 partials ⚠️
src/NRedisStack/Json/DataTypes/KeyValuePath.cs	75.00%	0 Missing and 1 partial ⚠️
src/NRedisStack/Search/AggregationRequest.cs	99.11%	0 Missing and 1 partial ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #241      +/-   ##
==========================================
- Coverage   94.37%   94.35%   -0.02%     
==========================================
  Files          88       88              
  Lines        5461     5425      -36     
  Branches      506      498       -8     
==========================================
- Hits         5154     5119      -35     
+ Misses        181      180       -1     
  Partials      126      126

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

shacharPash

Thank you!
A lot of good changes! 👍 👍
I have a question, some of the changes you made are also relevant to the other modules (topK, search, cuckooFilter, etc.), is there a reason why you didn't make changes there?

shacharPash · 2024-01-28T15:22:59Z

src/NRedisStack/Json/IJsonCommandsAsync.cs

@@ -157,7 +157,7 @@ public interface IJsonCommandsAsync
    /// <param name="value">The value to increment by.</param>
    /// <returns>The new values after being incremented, or null if the path resolved a non-numeric.</returns>
    /// <remarks><seealso href="https://redis.io/commands/json.numincrby"/></remarks>
-    Task<double?[]> NumIncrbyAsync(RedisKey key, string path, double value);
+    Task<double?[]> NumIncrByAsync(RedisKey key, string path, double value);


Its breaking change

The sync command still called NumIncrby

Thank you for the feedback you gave me and the time you took to review
This is valuable for me
Actually, when I applied these changes, it was after work and I was very tired
I was reviewing the codes of this project out of curiosity and made some changes
To be honest, I am very interested in participating and contributing to this project
I will fix the problems you mentioned and I would be grateful if you check and give me feedback again
I can fix all the parts you mentioned
We can also communicate via email or wherever you prefer

Hi Nima, I pushed a small commit reverting these ToString() calls. It was easiest, given that it's a super small change. Thanks for your patience!

Improve the signatures of the new methods, to transparently replicate the structure of the Redis command, i.e. timeout is mandatory and the first argument, order is mandatory. Add a version of the method that works for a single argument, in the spirit of the StackExchange.Redis library.

Add a dedicated enum for the MIN and MAX arguments. It feels cleaner than using Order and having to mentally map Ascending to Min and Descending to Max.

The primary constructor syntax seems to be not supported by older dotnet versions, so don't use it. Also switch from `class` to `struct`, to better reflect the fact that RedisValueWithScore is just a data storage item.

It seems that workflows cancel each other out if they are in the same concurrency group: https://github.com/orgs/community/discussions/41518 Try to separate four workflows that run on PRs into separate concurency groups. Ideally this will avoid the cancelation. They should still cancel runs from previous pushes, each in their own group.

…ommandsAsync.cs

Collection literals are not supported by older dotnet versions, it seems, so stop using them for now.

Some tests (for the Gears module) seem flaky, they fails sometime with RedisTimeoutException. Try to double the timeout in the connection multiplexer, from 5 to 10 seconds, hopefully this helps.

shacharPash · 2024-03-03T13:28:02Z

Hi @nimanikoo how are you?, regarding the missing modules - if you run the docker in the README (docker run -p 6379:6379 --name redis-stack redis/redis-stack:latest) you should get all the modules. I would appreciate it if you could specify what exactly you ran/downloaded so that we can understand why you were missing modules.

Secondly, the tests that run on .NET 6 and 7 still fail... because of a syntax that is only supported by .NET 8, for example: List<object> args = [key];

@nimanikoo, did you see my comment?

nimanikoo · 2024-03-03T19:10:41Z

Hi dear @shacharPash , how are you? I read your comment and this conflict was on my local machine. Redis was running locally on my machine on port 6379 and it did not have a number of Redis stack modules and it was causing problems, I fixed this local conflict and the problem was solved.Currently, all the tests are passed correctly, except the tests related to the search, which @omidpakdel mentioned, and I have the same problem in branch Master for tests.

to previous version becuase the old version of .net does not support the .net 8 syntax

shacharPash · 2024-03-12T09:46:09Z

Hey @nimanikoo, how are you?
What remains to be done is to delete the changes that have the syntax supported only in .NET 8 and not in the previous versions of .NET and then we can merge
can you do it?

nimanikoo · 2024-03-12T13:52:34Z

Hi @shacharPash ,How are you doing?
I will definitely fix all the issues that you've identified and that are causing conflicts.
I would really appreciate it if you could please provide me with a detailed list of the specific issues that are causing the conflicts so that I can address them all thoroughly.

shacharPash · 2024-03-12T16:00:49Z

@nimanikoo You can see the problematic sections in what failed the .NET 6 and 7 tests

… constructor in .net 8

…llection literals'

…ror : error CS8652: The feature 'collection literals' is currently in Preview and *unsupported*. To use Preview features, use the 'preview' language version

nimanikoo · 2024-03-13T16:21:42Z

Hello @shacharPash , How are you doing?
I've fixed all the compiler errors that were there, and I'm confident that we won't have any more issues with the .NET version or syntax.
I would have really loved to be able to fully utilize the new features of .NET 8 in this project, but unfortunately, it's not possible.
Please let me know if there's anything else I can do to help us get closer to merging.

…tionWithoutGraph

…cs for readonly strings

…d Person.cs

gerzse

Looks good to me. Two minor comments.

gerzse · 2024-03-18T06:24:25Z

tests/Doc/StringSnippets.cs

@@ -27,7 +27,7 @@ public void run()

        // STEP_START set_get
        var res1 = db.StringSet("bike:1", "Deimos");
-        Console.WriteLine(res1); // true
+        Console.WriteLine(res1.ToString()); // true


Isn't ToString() implied here? These code snippets end up on the reddis.io web documentation, they should be as easy to read as possible.

Hello Gabriel,
How are you?
Thanks for the points you mentioned.
I checked with branch master and found that .ToString() wasn't implemented, but I don't think it would cause any issues since it just logs.
Your opinion is still a priority, and if you agree, we can make some changes.

gerzse · 2024-03-18T06:28:01Z

src/NRedisStack/Auxiliary.cs

+
+    public static async Task<RedisResult> ExecuteAsync(this IDatabaseAsync db, SerializedCommand command)
+    {
+        if (!_setInfo) return await db.ExecuteAsync(command.Command, command.Args);


In this class I find the original logic easier to read. Now the db.ExecuteAsync(...) call is duplicated, so if we need to change it, it needs change in two places.

It was a very good point. I fixed it. Thank you

…sole complexity

Trying to keep code snippets as short as possible, since they get published on redis.io as documentation. Hence reverting the addition of the `ToString()` calls in some places.

gerzse and others added 19 commits January 25, 2024 11:41

Formatting and typos

61011e3

Fix some typos in the markdown files and run `dotnet format` on existing code.

Include documentation in the build

07cdbcc

Change the settings so that documentation is included in the build.

Skip new tests for Redis Cluster

d5b8880

Refactor and Change the List Collection to ICollection for better per…

132a224

…formance and memory usage

Change NameSpace to Scoped

b82ef2a

Add Db to xml summary doc

5ffe8ad

Fix Typo problem

bdd0b79

Change constructor to primary constructor based on new feature in .net 8

cbfaa7b

Fix Type for previously

9ceafb6

refactor and Use Collection Expression for list initializing and chan…

ac81e90

…ge foreach to linq expression

use primary constructor in CmsCommands.cs

24ad2aa

use primary constructor in CmsCommands.cs

d2f877a

Merge remote-tracking branch 'origin/Fixup' into Fixup

dbabb3a

Refactor and change constructor to primary

3cc5221

Fix Typo and indent in CommandArgs.cs and IJsonCommands.cs

fd45c00

Fix Typo in ExampleTests.cs and IJsonCommandsAsync.cs and JsonCommand…

0713820

…sAsync.cs and JsonTests.cs

Use Linq and List Expression

32d83d4

Refactor

bac3515

shacharPash reviewed Jan 28, 2024

View reviewed changes

gerzse and others added 9 commits January 29, 2024 12:22

Add test for BzmPop without keys

0a8047a

Add dedicated enum for MIN/MAX

bbe2c4a

Add a dedicated enum for the MIN and MAX arguments. It feels cleaner than using Order and having to mentally map Ascending to Min and Descending to Max.

Remove primary constructor syntax

117207c

The primary constructor syntax seems to be not supported by older dotnet versions, so don't use it. Also switch from `class` to `struct`, to better reflect the fact that RedisValueWithScore is just a data storage item.

Fix indent in CuckooInformation.cs

b8cd5d6

RollBack to original version of naming NumIncrbyAsync method in JsonC…

bc36846

…ommandsAsync.cs

Remove use of collection literals

5bbf60f

Collection literals are not supported by older dotnet versions, it seems, so stop using them for now.

Increase multiplexer timeout in tests

bbb97d6

Some tests (for the Gears module) seem flaky, they fails sometime with RedisTimeoutException. Try to double the timeout in the connection multiplexer, from 5 to 10 seconds, hopefully this helps.

nimanikoo requested a review from shacharPash March 3, 2024 11:41

shacharPash added the maintenance label Mar 3, 2024

nimanikoo and others added 5 commits March 3, 2024 22:56

Roleback ' List<object> args = [key];'

ecf7ae0

to previous version becuase the old version of .net does not support the .net 8 syntax

Merge branch 'master' into Fixup

a9ad102

Merge branch 'master' into pr/nimanikoo/241

6266db5

fix compilation error

30ff30b

format

a3eb232

Merge branch 'master' into Fixup

15eaca6

nimanikoo added 4 commits March 12, 2024 21:43

Fix Indent and Solve Problem of old .net version with feature primary…

d35f866

… constructor in .net 8

Merge remote-tracking branch 'origin/Fixup' into Fixup

c9f3cac

Fix Issues for compiler error in .net 8 error CS8652: The feature 'co…

5817619

…llection literals'

Fix Compiler Error in JsonCommandsAsync.cs and JsonCommands.cs for Er…

496fcc3

…ror : error CS8652: The feature 'collection literals' is currently in Preview and *unsupported*. To use Preview features, use the 'preview' language version

shacharPash and others added 6 commits March 14, 2024 17:24

return ExampleTests's ctor

56ca7ae

Fix typo for TransactionsTests.cs in method naming TestModulesTransac…

1927c87

…tionWithoutGraph

Fix Typo and namespace also Set Private Access Modifier RedisFixture.…

e1eacb3

…cs for readonly strings

Fix Typo and set const for readonly key PipelineTests.cs

d75b17b

Fix Typo and Change Namespace to scoped AbstractNRedisStackTest.cs an…

15e3e44

…d Person.cs

Return to Collection Initializer for collections in ExampleTests.cs

5bb7d65

gerzse reviewed Mar 18, 2024

View reviewed changes

Return ExecuteAsync method in Auxiliary.cs to original version for …

ef9ddaa

…sole complexity

nimanikoo requested a review from gerzse March 18, 2024 07:12

Revert ToString() in code snippets

4159b23

Trying to keep code snippets as short as possible, since they get published on redis.io as documentation. Hence reverting the addition of the `ToString()` calls in some places.

gerzse approved these changes Mar 19, 2024

View reviewed changes

shacharPash merged commit fc40221 into redis:master Mar 19, 2024
18 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhance code readability #241

Enhance code readability #241

nimanikoo commented Jan 26, 2024

codecov-commenter commented Jan 28, 2024 •

edited

Loading

shacharPash left a comment

shacharPash Jan 28, 2024

nimanikoo Jan 29, 2024

gerzse Mar 19, 2024

shacharPash commented Mar 3, 2024

nimanikoo commented Mar 3, 2024

shacharPash commented Mar 12, 2024

nimanikoo commented Mar 12, 2024

shacharPash commented Mar 12, 2024

nimanikoo commented Mar 13, 2024

gerzse left a comment

gerzse Mar 18, 2024

nimanikoo Mar 18, 2024 •

edited

Loading

gerzse Mar 18, 2024

nimanikoo Mar 18, 2024

Enhance code readability #241

Enhance code readability #241

Conversation

nimanikoo commented Jan 26, 2024

codecov-commenter commented Jan 28, 2024 • edited Loading

Codecov Report

shacharPash left a comment

Choose a reason for hiding this comment

shacharPash Jan 28, 2024

Choose a reason for hiding this comment

nimanikoo Jan 29, 2024

Choose a reason for hiding this comment

gerzse Mar 19, 2024

Choose a reason for hiding this comment

shacharPash commented Mar 3, 2024

nimanikoo commented Mar 3, 2024

shacharPash commented Mar 12, 2024

nimanikoo commented Mar 12, 2024

shacharPash commented Mar 12, 2024

nimanikoo commented Mar 13, 2024

gerzse left a comment

Choose a reason for hiding this comment

gerzse Mar 18, 2024

Choose a reason for hiding this comment

nimanikoo Mar 18, 2024 • edited Loading

Choose a reason for hiding this comment

gerzse Mar 18, 2024

Choose a reason for hiding this comment

nimanikoo Mar 18, 2024

Choose a reason for hiding this comment

codecov-commenter commented Jan 28, 2024 •

edited

Loading

nimanikoo Mar 18, 2024 •

edited

Loading