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

feat: use serde for de/serializing GuildId #206

Merged
merged 4 commits into from
Nov 2, 2023
Merged

Conversation

seqre
Copy link
Contributor

@seqre seqre commented Oct 31, 2023

Serde is smart and can handle a lot of de/serializing magic. This PR aims to reduce extraneous manual implementations in favor of Serde's existing functionality.

I know deserializing works in this case (Serde is fully capable of taking a String as input and going through multiple transformations of String -> u64 -> GuildId); however, I need to do more testing for serializing (some additional Serde magic might be needed to force it to serialize to String, nothing though that Serde shouldn't be capable of); therefore, this PR is in draft mode for now.

@seqre seqre self-assigned this Oct 31, 2023
@seqre
Copy link
Contributor Author

seqre commented Oct 31, 2023

Funny thing, disregarding the fact Serde deserializes to numbers (shown below), the tests pass, and config loads lol

[guilds.420]
tournaments_watcher_channel_id = "69"

[guilds.69]
tournaments_watcher_channel_id = "2137"

@codecov
Copy link

codecov bot commented Oct 31, 2023

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (c119696) 43.56% compared to head (5bff7fd) 45.38%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #206      +/-   ##
==========================================
+ Coverage   43.56%   45.38%   +1.82%     
==========================================
  Files          22       22              
  Lines        1398     1430      +32     
==========================================
+ Hits          609      649      +40     
+ Misses        789      781       -8     
Flag Coverage Δ
rust 45.38% <95.65%> (+1.82%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
chombot/src/config.rs 89.74% <100.00%> (+13.27%) ⬆️
chombot/src/tournament_watcher.rs 0.00% <0.00%> (ø)

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

@seqre
Copy link
Contributor Author

seqre commented Oct 31, 2023

Actually, digits are valid keys for TOML tables/dictionaries:

toml-rs seems to support only String as the Table key, but somehow it works.

@seqre seqre force-pushed the config_serde_improvements branch from 66e1f07 to ee38171 Compare November 2, 2023 23:23
@seqre seqre marked this pull request as ready for review November 2, 2023 23:45
@seqre seqre requested review from m4tx and Iipin November 2, 2023 23:45
@seqre
Copy link
Contributor Author

seqre commented Nov 2, 2023

It just works ™️

@m4tx
Copy link
Member

m4tx commented Nov 2, 2023

Thanks for the nice simplification!

@seqre seqre merged commit 1f8c1f5 into master Nov 2, 2023
7 checks passed
@seqre seqre deleted the config_serde_improvements branch November 2, 2023 23:56
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.

2 participants