Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

✨ Add Value helpers and trait impls #26

Merged
merged 1 commit into from
Dec 7, 2024

Conversation

CosmicHorrorDev
Copy link
Contributor

@CosmicHorrorDev CosmicHorrorDev commented Dec 6, 2024

Stemming from #23 (comment)

Adds (eliding lifetimes for brevity)

  • Value::as_str(&self) -> Option<&str>
  • Value::as_i64(&self) -> Option<i64>
  • Value::as_f64(&self) -> Option<f64>
  • Value::as_bool(&self) -> Option<bool>
  • Value::as_array(&self) -> Option<&Array>
  • Value::as_table(&self) -> Option<&Table>
  • impl From<X> for Value
    • String
    • &str
    • Cow<str>
    • i64
    • f64
    • bool
    • Array
    • Table
  • impl<V: Into<Value>> FromIterator<V> for Value (arrays)
  • impl<K: Into<Cow<str>>, V: Into<Value>> FromIterator<(K, V)> for Value (tables)

There's a decent loc reduction, but also significant line-noise reduction in the tests that were manually constructing Values


I briefly started adding PartialEq<T> impls for Value, but then I quickly realized just how many different combinations would be required to cover all the common cases, and instead I noped out and leaned into the conversion helpers more

@zeenix
Copy link
Owner

zeenix commented Dec 6, 2024

  • Value::as_str(&self) -> Option<&str>
  • Value::as_i64(&self) -> Option<i64>
  • Value::as_f64(&self) -> Option<f64>
  • Value::as_bool(&self) -> Option<bool>
  • Value::as_array(&self) -> Option<&Array>
  • Value::as_table(&self) -> Option<&Table>

Out of coincidence, I also added conversions in my WIP branch but I went for Tryfrom impls instead and also used a macro to reduce the LOC. I think I prefer my approach this really is a conversion and *From impls are meant for that. Moreover, that enables generic code like I'm trying to add in the same commit.

I'll split the conversions into a separate PR soon so you can rebase on top of that. I really should have done that already but I didn't know that I'll hit a wall with a lifetime issue (try building that branch if you're curious).

@CosmicHorrorDev
Copy link
Contributor Author

I think I prefer my approach this really is a conversion and *From impls are meant for that. Moreover, that enables generic code like I'm trying to add in the same commit.

We can do both if you want, but my code is following the general ad-hoc conversions which is still certainly idiomatic rust (and also mimics the APIs you see in serde_json, toml, etc.)

The main reason I don't want to drop the .as_*() helpers is that you can't turbofish .try_into() so a lot of the method-chainy code gets a lot messier without it

@zeenix
Copy link
Owner

zeenix commented Dec 6, 2024

my code is following the general ad-hoc conversions which is still certainly idiomatic rust

That guideline is about naming of such conversion methods and doesn't say anything about preferring these methods over implementing the conversion traits.

(and also mimics the APIs you see in serde_json, toml, etc.)

That's a good argument though. :)

We can do both if you want,

Yeah, sure. Let's do that. :) Should I merge then or you want to review the changes?

@CosmicHorrorDev
Copy link
Contributor Author

You can take over this PR. I don't think I'll have enough spare time to be able to contribute here more

@zeenix
Copy link
Owner

zeenix commented Dec 7, 2024

I don't think I'll have enough spare time to be able to contribute here more

I hope everything is ok? It's very sad and surprising to hear, given that I've changed the main goals of the project twice already, based on your promise to help develop and maintain this crate and now you'll be disappearing completely so soon. 😔

@CosmicHorrorDev
Copy link
Contributor Author

Eh it's just life deciding to be busy again, so I won't have much free time to dedicate to OSS for the next few months at least unfortunately

All of my other projects are in a pretty stable state, but this one being new means it would demand a lot more attention than I'll be able to give

@zeenix
Copy link
Owner

zeenix commented Dec 7, 2024

Eh it's just life deciding to be busy again, so I won't have much free time to dedicate to OSS for the next few months at least unfortunately

Ah ok. I appreciate you being so honest about it. Most folks keep promising to finish their PRs but never do. :(

Co-authored-by: Zeeshan Ali Khan <zeenix@gmail.com>
@zeenix
Copy link
Owner

zeenix commented Dec 7, 2024

You can take over this PR.

Done. Thanks.

@zeenix zeenix merged commit 9798627 into zeenix:main Dec 7, 2024
3 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants