Skip to content

chore: fix unnecessary usage of () in Redis commands #600

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

Closed
wants to merge 1 commit into from

Conversation

defiberrys
Copy link

I found an issue where () was incorrectly used in the following lines:

() = conn.set(&key, data.clone()).await?;

and

() = conn.publish(&key, "updated").await?;

This usage of () was not needed, as it doesn't serve any purpose here. The code has been corrected to simply call the methods without the unnecessary assignment:

conn.set(&key, data.clone()).await?;

and

conn.publish(&key, "updated").await?;

These changes ensure that the Redis commands are executed correctly.

Copy link
Collaborator

@benalleng benalleng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sure to run the linter with ./contrib/lint.sh when pushing changes, we have the flag -D warnings set so any warning is considered a failure!
I found this warning.

warning: this function depends on never type fallback being `()`
  --> payjoin-directory/src/db.rs:74:5
   |
74 | /     async fn push(
75 | |         &self,
76 | |         subdirectory_id: &ShortId,
77 | |         channel_type: &str,
78 | |         data: Vec<u8>,
79 | |     ) -> Result<()> {
   | |___________________^
   |
   = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in Rust 2024 and in a future release in all editions!
   = note: for more information, see <https://doc.rust-lang.org/nightly/edition-guide/rust-2024/never-type-fallback.html>
   = help: specify the types explicitly
note: in edition 2024, the requirement `!: FromRedisValue` will fail
  --> payjoin-directory/src/db.rs:82:14
   |
82 |         conn.set(&key, data.clone()).await?;
   |              ^^^
   = note: `#[warn(dependency_on_unit_never_type_fallback)]` on by default

I think we can find a cleaner way to handle ignoring the Result here but perhaps there was more thought to the inclusion of these () than it might have seemed.

@coveralls
Copy link
Collaborator

Pull Request Test Coverage Report for Build 14007606865

Details

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.03%) to 80.378%

Files with Coverage Reduction New Missed Lines %
payjoin/src/send/mod.rs 2 94.31%
Totals Coverage Status
Change from base Build 14002185069: -0.03%
Covered Lines: 4846
Relevant Lines: 6029

💛 - Coveralls

@DanGould
Copy link
Contributor

I think we can find a cleaner way to handle ignoring the Result here

The syntax using ? means that the Result is explicitly handled by propagating an error and returning the unit type for Ok. I don't think I'd describe that as being ignored

@benalleng
Copy link
Collaborator

benalleng commented Mar 24, 2025

Thats a fair clarification, I guess we are just constraining the type with () = TBH I was a little confused about what the warning was trying to tell us.., If I had to guess I'm going to assume we've already gone through all of these resources already seeing as spacebear already made this commit 219296f but I figure they are a good read to clarify why this code is the way it is.

https://users.rust-lang.org/t/this-function-depends-on-never-type-fallback-being/120158/6

rust-lang/rust#126466

rust-lang/rust#123748 (comment)

https://doc.rust-lang.org/edition-guide/rust-2024/never-type-fallback.html

Long story short the usage of ? makes this code infer to either a () or ! depending on the never type fallback. Because our code expects returning the RedisResult with a generic T we should set it to be () = conn.set(&key, data.clone()).await?; so that we don't fallback to the never ! type.

@DanGould DanGould closed this Mar 24, 2025
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.

4 participants