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

Update dependencies & stricter clippy #1

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

mladedav
Copy link

@mladedav mladedav commented Aug 29, 2024

Much stricter clippy and thanks to that getting rid of some potential panics, especially when indexing strings obtained from users.

Also some dependency upgrades. I did just those that went (mostly) smoothly, there have been like 20 azure releases since this was written and I don't really have the bandwidth to dig into that.

There are still unwraps and expects left and those should also be fixed where possible when someone gets around to do that.

Many of those are getting rid of potential panics due to malformed connection
strings, IDs and other user input.
Copy link
Collaborator

@roberthusak roberthusak left a comment

Choose a reason for hiding this comment

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

Wow, that's a lot of improvements, thanks! 👍 How could have I lived without let-else 🤩

I just added some minor comments and questions.

@@ -0,0 +1,2 @@
[toolchain]
channel = "1.80.1"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there any advantage of setting the toolchain to a fixed version? We currently always use the current version in CI (i.e. the version installed on the current runner) and AFAIK it works quite well. For example, we don't have to think about updating the toolchain manually. But maybe there's something I'm missing.

.expect("Unreachable because we have checked the number of parts");
let Some(properties) = parts[4].strip_prefix('?') else {
bail!("Received message with malformed properties '{}'.", parts[4]);
};

// Skip leading question mark
Copy link
Collaborator

Choose a reason for hiding this comment

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

This comment can be deleted now.

@@ -156,7 +162,7 @@ mod tests {
}

#[test]
#[should_panic]
#[should_panic(expected = "This duration cannot be deserialized.")]
Copy link
Collaborator

Choose a reason for hiding this comment

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

This tests fails because the string "This duration cannot be deserialized." isn't a substring of the actual error message. I suggest either changing the string to "Duration did not contain two colons." or removing it.

log = "0.4.16"
native-tls = "0.2.8"
openssl = { version = "0.10.29", optional = true }
rumqttc = { package = "spotflow-rumqttc-fork", version = "0.12.0", features = ["use-native-tls"], default-features = false }
serde = { version = "1.0.136", features = ["derive"] }
serde_json = "1.0.79"
sqlx = { version = "0.6.2", features = ["sqlite", "chrono", "macros", "runtime-tokio-native-tls", "offline"] }
sqlx = { version = "0.7.4", features = ["sqlite", "chrono", "macros", "runtime-tokio", "tls-native-tls"] }
Copy link
Collaborator

Choose a reason for hiding this comment

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

I saw that sqlx changed the format of its offline query metadata - instead of using the single file sqlx-data.json they now save the queries as individual files to the .sqlx directory. Therefore, the CI build fails now. Can you please run cargo sqlx prepare in the spotflow directory (you'll probably need to run cargo install [email protected] before that) and then commit all the generated files? And also delete sqlx-data.json?

Btw. I'd like to investigate if it wouldn't be easier to just use dev.db on CI as well so that we could get rid of calling cargo sqlx prepare altogether. Do you think it would work? Or are there any problems you encountered when working on this earlier?

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

Successfully merging this pull request may close these issues.

2 participants