Skip to content

Conversation

@JoeJoeTV
Copy link
Contributor

@JoeJoeTV JoeJoeTV commented Jan 6, 2025

As mentioned in #9, I added functionality to provide the dispatcher builder and the Async/Blocking structs with a custom client/agent builder, so things like the user agent, which is not handled by the dispatcher builder already can be set beforehand.

I added this in the form of extra functions that take an additional argument.
If I should take a different approach, I can do that.

EDIT: This should not break anything as far as I can see, because it just adds additional functions.

@yukibtc yukibtc closed this in 0a058cb Jan 8, 2025
@yukibtc
Copy link
Member

yukibtc commented Jan 8, 2025

Thanks! Cherry-picked and squashed at 0a058cb

@yukibtc
Copy link
Member

yukibtc commented Jan 8, 2025

@JoeJoeTV I think we are ready for v0.7? Or have you other ideas for changes/improvements?

@JoeJoeTV
Copy link
Contributor Author

JoeJoeTV commented Jan 8, 2025

One thing that would be nice, is adding support for the serde Serialize and Deserialize traits in e.g. the Priority enum, as currently, I gave a copy of that type in my application to be able to load the configuration.
I think most crates have a "serde" feature for this.

yukibtc added a commit that referenced this pull request Jan 8, 2025
@yukibtc
Copy link
Member

yukibtc commented Jan 8, 2025

One thing that would be nice, is adding support for the serde Serialize and Deserialize traits in e.g. the Priority enum, as currently, I gave a copy of that type in my application to be able to load the configuration. I think most crates have a "serde" feature for this.

Adj. here: 7df38ea

@JoeJoeTV
Copy link
Contributor Author

JoeJoeTV commented Jan 8, 2025

Adj. here: 7df38ea

Hmm, this serializes the enum to an i32, personally I was looking more into (de-)serializing it into strings, like "max", "hight", etc., so it could easily be set e.g. in a config file as a string.
If you want to also keep the option to (de-)serialize this as an integer, maybe there is a way to have both possibilities in serde?
What you you think?

@yukibtc
Copy link
Member

yukibtc commented Jan 8, 2025

Adj. here: 7df38ea

Hmm, this serializes the enum to an i32, personally I was looking more into (de-)serializing it into strings, like "max", "hight", etc., so it could easily be set e.g. in a config file as a string.
If you want to also keep the option to (de-)serialize this as an integer, maybe there is a way to have both possibilities in serde?
What you you think?

I'm de/serializing to a u8 because, according to the ntfy docs, the priority it's an number: https://docs.ntfy.sh/publish/#message-priority

You can do custom de/serialization in your code. For example, wrapping the Priority enum in a struct and impl manually the serde traits.

EDIT: in the curl example seems that it use the priority also as string, but I prefer the u8 way because more performant than strings. I'll check anyway if there is a way to support both

@JoeJoeTV
Copy link
Contributor Author

JoeJoeTV commented Jan 8, 2025

You can do custom de/serialization in your code. For example, wrapping the Priority enum in a struct and impl manually the serde traits.

I am currently doing it using the derive for Serialize and Deserialize, but the easiest way to do this with an external enum which is optional in a struct and serde derive is to basically just copy the enum definition and derive the traits in addition to implementing From and Into for the actual enum.
But that is a bit annoying, as I have to copy the definition just for this.
Thanks for checking though!

@JoeJoeTV
Copy link
Contributor Author

@yukibtc Maybe you could also use the is_human_readable function for (de-)serializers (or only one of them) to determine whether to (de-)serialize to a string or a u8. THis would also cover my case, since formats like toml or yaml, which are used as config file formats are marked as human readable, while e.g. binary formats like BSON, or DBus variants (just to name some example) have that method return false?

I'm curious though: In which way is perfomance important for you regarding this value, since the content is sent as JSON anyways?

@yukibtc
Copy link
Member

yukibtc commented Jan 12, 2025

@yukibtc Maybe you could also use the is_human_readable function for (de-)serializers (or only one of them) to determine whether to (de-)serialize to a string or a u8. THis would also cover my case, since formats like toml or yaml, which are used as config file formats are marked as human readable, while e.g. binary formats like BSON, or DBus variants (just to name some example) have that method return false?

Thanks, I saw this option but I didn't tried it yet. Will try it soon.


I'm curious though: In which way is perfomance important for you regarding this value, since the content is sent as JSON anyways?

The difference isn't huge and anyway the highest (when serializing the string) is still pretty low. Probably for this library this attention to performance is not necessary, but I always prefer to use the most efficient way.

Serialization of the payload as JSON, with the priority serialized as string:

test payload::benches::serialize_payload ... bench:         211.86 ns/iter (+/- 101.02)

Serialization of the payload as JSON, with the priority serialized as u8:

test payload::benches::serialize_payload ... bench:         144.87 ns/iter (+/- 35.42)

@JoeJoeTV
Copy link
Contributor Author

JoeJoeTV commented Jan 12, 2025

The difference isn't huge and anyway the highest (when serializing the string) is still pretty low. Probably for this library this attention to performance is not necessary, but I always prefer to use the most efficient way.

Serialization of the payload as JSON, with the priority serialized as string:

test payload::benches::serialize_payload ... bench:         211.86 ns/iter (+/- 101.02)

Serialization of the payload as JSON, with the priority serialized as u8:

test payload::benches::serialize_payload ... bench:         144.87 ns/iter (+/- 35.42)

I see the difference and I understand the want for the biggest possible performance, but I'm not sure that the serialize performance here will be the bottleneck in many situations.
But alas, maybe an option you would be more comfortable with, would be to just implement this for the deserialize trait, since this crate currently doesn't receive anything from ntfy, so deserializing would only be required for loading stuff from configs, converting from some other format, etc.

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