-
Notifications
You must be signed in to change notification settings - Fork 69
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
Rust - code cleanup, part 1 #2478
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, this file should not be kept in the respository
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, but for old reasons it's on the part and I don't want to break any one work by completely remove it.
The specific line here is needed for analyzer to understand how to read complicated monorepo, and many had tackle the issue, so we added a shared solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this file is still being maintained at all, can we please consider to remove all the OnSave
settings in it? Currently, I'm using:
git update-index --skip-worktree .vscode/settings.json
to prevent the OnSave
setting to be reverted to on every time I pull or switch branches, as not only it is in the git repo, but the entire .vscode
folder is also in .gitignore
, making any change you made ignored by git.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's not break anything to remove it, we should.
But it will affect a lot of us, so I think we should be careful with it.
Try to remove it and see the effect, and if no effect we can try in a few other WS and see if it is feasible to remove and not break everyone's WS. I'd be happy if we got rid of it, each can use his config.
b6c38ef
to
28048ec
Compare
Signed-off-by: avifenesh <[email protected]>
28048ec
to
b6629b0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please rename the PR and add changelog
git submodule update --init --recursive | ||
``` | ||
3. Install all node dependencies: | ||
2. Install all node dependencies: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix this for all clients please
What do you mean by rename pr? I think change log are for what relevant in release, no? Its not including changes that has no connection to users |
Well, I guess "Rust - code cleanup, part 1" is better
Not sure, need to confirm with @ikolomi. CI changes have no effect for user too, but we list them there. |
Yes my question is on CI as well, are we want to include it in release note? Im not sure its relevant |
Some cleaning for unnecessary code to set the floor for next PR Signed-off-by: avifenesh <[email protected]>
Some cleaning for unnecessary code to set the floor for next PR Signed-off-by: avifenesh <[email protected]>
Some cleaning for unnecessary code to set the floor for next PR Signed-off-by: avifenesh <[email protected]>
Some cleaning for unnecessary code to set the floor for next PR Signed-off-by: avifenesh <[email protected]>
This pull request includes various changes across multiple files to improve code readability, update configurations, and remove deprecated methods.
Configuration Updates:
.vscode/settings.json
: Updated the path forredis-rs/Cargo.toml
and added new settings formakefile
andrust-analyzer
. [1] [2]Dependency and Feature Formatting:
glide-core/Cargo.toml
: Reformatted dependency and feature lists for better readability. [1] [2]glide-core/redis-rs/redis/Cargo.toml
: Reformatted feature lists and added new metadata. [1] [2] [3]Code Refactoring:
glide-core/redis-rs/redis/src/aio/connection.rs
: Removed themap
method and refactored theconnect
function. [1] [2]glide-core/redis-rs/redis/src/client.rs
: Removed deprecated async connection methods. [1] [2] [3]Documentation and Testing:
node/DEVELOPER.md
: Updated the steps for building and testing the Node wrapper. [1] [2] [3]node/tests/SharedTests.ts
: Replaced deep equality to deep close check withexpect.arrayContaining
andexpect.closeTo
for handling precision diff in floats.Additional Changes:
glide-core/redis-rs/redis/src/cluster_async/connections_logic.rs
: Replaced custom timeout logic withtokio::time::timeout
. [1] [2]glide-core/redis-rs/redis/src/types.rs
: Added a conversion fromtokio::time::error::Elapsed
toRedisError
.Please make sure you are aware of our contributing guidelines available
here
Issue link
This Pull Request is linked to issue: [#2450 ]
Checklist
Before submitting the PR make sure the following are checked: