Update dependencies, remote unnecessary RwLock on database, fix typos #43
silver
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "cordlesscoder/discord-bot:main"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
@ -163,3 +153,1 @@//? maybe handle this and possibly throw error if its nonelet _ = resvg::render(&tree, usvg::FitTo::Size(self.size.0, self.size.1), tiny_skia::Transform::default(), pixmap.as_mut());resvg::render(&tree, usvg::Transform::default().post_scale(scale, scale), &mut pixmap.as_mut());Please double check this replacement logic for FitTo
Read the top comment on this page, then decide what ye want to do.
Overall its pretty good.
You clearly dont like typos though.
Seems like updating the dependencies lead ye to write code ye didn't understand.
Can ye stand over it?
If ye cant then have a think about it.
Honestly would be really nice to have the rwlock removed, at teh time I was writing it originally it was shouting at me until I added it.
Good to see that it has changed since.
.unwrap()is useful for testing but having it in prod leads to not correctly handling the errors.@ -7,4 +7,5 @@ fn_params_layout = "Compressed"struct_lit_width = 0tab_spaces = 2use_small_heuristics = "Max"imports_granularity = "Crate"Why disable this?
It isn't available in stable rustfmt, it's still a nightly feature
I am aware, throws out some logging if ye are on stable, but does not cause any errors in pipeline
@ -22,3 +22,3 @@[dependencies]# discord libraryserenity = { version = "0.12", default-features = false, features = ["client", "gateway", "rustls_backend", "model", "cache"] }serenity = { version = "0.12", default-features = false, features = [Dont do this.
More specifically if ye want it to look prettier then ye use the alternate format, not formatting it like this.
I didn't do this, my TOML formatter did(taplo). That's the dark side of enabling format on save :(
Not quite sorted yet, for a hint as to why the single line stuff is better for this use case look at
wolves_oxidized.@ -48,3 +56,3 @@maud = "0.27"toml = "0.8.23"toml = "0.9.5"Is there any reason to update?
https://github.com/toml-rs/toml/blob/main/crates/toml/CHANGELOG.md#092---2025-07-11
That fixes an infinite loop on certain invalid TOML inputs
from the 9.0 notes:
joys of a major bump, so not really a too valid answer for this case.
Not to mention bumping teh image dependencies brought its own issues.
The major bump happened 2 months ago and the TOML crate is ridiculously popular - I believe it's reasonable enough to assume that any regressions have been found and fixed.
The image dependencies were 2 years out of date and therefore missing support for many SVG features, e.g. color emojis.
For the SVG converter I think I mentioned elsewhere, but was it broken?
Did it actually need to be updated?
What problem did it solve?
It wasn't broken, it was simply missing support for many SVG features - maybe the logos it deals with don't use them, in which case it makes no difference.
But the change is like 10 lines of logic and makes sure we have improved SVG support. If you don't think that's worth it, you're free to not accept the change.
@ -42,3 +42,3 @@data.insert::<Config>(Arc::new(RwLock::new(config)));data.insert::<DataBase>(Arc::new(RwLock::new(db)));data.insert::<DataBase>(Arc::new(db));Out of curiosity did ye try and run the bot, or did ye entirely depend on fmt/clippy?
Asking as when first set up it needed to be using the rwlock, things might have changed since though.
The RwLock can't be required, as the only extra capability it adds is getting a mutable reference to the Database - something your code doesn't do once, and something sqlx's Pool intentionally avoids.
Just be cause it is not now does not mean it never was.
Still set of comments was more remarking on it than actually needing it changed, so marking it resolved
@ -109,3 +109,1 @@if let Ok(x) = x.trim().parse::<u64>() {config.committee_role = RoleId::new(x);}let x = x.trim().parse().unwrap();Dont do this, its like saying that ye hate rust.
The original code instead ignores errors - that is not better.
The error is invalid configuration, there is no reasonable way to recover from an error like this - the only reasonable course of action is to panic.
Please elaborate on how it is
the only reasonable course of action is to panicThis is an env flag set on deployment. It will only fail if you provide an invalid value for configuration.
So, the situation is: the discord bot has been started with improper configuration. What should we do?
Should we run and silently ignore invalid values, or should we alert whoever is responsible for the deployment?
Sure, you could say we can just print an error instead - but that is far less visible than the bot simply refusing to run if the configuration you provide doesn't make sense.
If we were running the bot for just ourselves then that logic would make sense.
However we provide it as a service to other clubs and societies, heck even that segment is for the committee server, which is not its core duty.
If it fails to parse the config values it will most likely remove teh committee role from committee members, which we will find out about pretty fast, but the normal users on the different discords would remain not impacted.
So not properly handling the error is not a valid choice ehre.
(Pretty impressed on how ye are picking up on the code as a whole)
Alright, that seems reasonable. Thanks for the explanation.
What leads you to say I'm writing code I don't understand? I made every change that has semantic significance with intent, and I can justify it just fine.
Its not a big deal but maybe look into this for your typo commit, as it would make gitblame a little more useful. If someone in the future wanted to ask a follow up on a comment it would look like it was your comment instead of silver's.
Also check the ci/cd pipelines they seem to be failing
@cordlesscoder wrote in #43 (comment):
#43 (comment)
Can ye say its a proper replacement?
How do ye know?
Why change somethign that is (literally) not broken?
@silver wrote in #43 (comment):
FitTo was removed over 2 years ago. I can say that logically it's a proper replacement, but I have not tested it so I just asked to do that, as that's the change I think is most likely to be incorrect.
@ -0,0 +1,2 @@# Fix typos4b4e5cb2894346684034cba93a5ac1ec6f884f9f:3 I really like this (not too many folks know about this)
@ -7,4 +7,4 @@ fn_params_layout = "Compressed"struct_lit_width = 0tab_spaces = 2use_small_heuristics = "Max"imports_granularity = "Crate"this diff may confuse folks in the future, but its fine
@ -30,2 +29,3 @@# to make the http requests[dependencies]wolves_oxidised = { git = "https://forgejo.skynet.ie/Skynet/wolves-oxidised.git", features = ["unstable"] }Fix it properly, that includes teh commented out one
@ -115,3 +115,1 @@if let Ok(x) = part.trim().parse::<u64>() {config.committee_category.push(ChannelId::new(x));}let Ok(x) = part.trim().parse() else { continue };Fix it properly, this just changes it for teh sake of changing it
The logic is identical - in what way is this "improper", it's just less nested.
Pull request closed