onlivfe / vrc_rs

VRChat's API in a rust idiomatic way (unofficial)
Mozilla Public License 2.0
6 stars 2 forks source link

Add Group Ban/Unban, SearchUser, and GroupMember queries/models #8

Closed ShayBox closed 3 months ago

ShayBox commented 6 months ago

I recommend a squash merge into one commit.

ljoonal commented 6 months ago

I'm currently quite busy for a few more weeks, but I'll try to remember to take a look at this when I have a bit more of time :sweat_smile:

ShayBox commented 6 months ago

For Option<OffsetDateTime>, scourging the rest of the code revealed that there should already be easy-to-add support for it in the crate;

  #[serde(
      default,
      deserialize_with = "crate::deserialize_optional_date",
      serialize_with = "rfc3339::option::serialize"
  )]

I believe one or more of the fields didn't parse correctly as an OffsetDateTime, but it's been so long since I made these changes prior to submitting this pull request that I don't remember which one(s).

I'm seemingly the only person using this library right now, likely because the coverage of the API is so low (less than 10%) that it's not worth peoples time to create entire requests/responses and then have to validate and test every field to get it submitted.

I'm just converting a single response with JSON -> Serde and using ChatGPT to add comments until it covers the APIs I need for my single project. I believe this library would have more contributors if that was done to all the responses to get 80-90% coverage, and it would be easier for users to amend types as needed.

I don't know what your intentions with this library are, but if you intend this to be used outside your own project(s) by the larger community, the coverage needs to grow before the existing vrchatapi-rust project fixes the few problems preventing it from being used (async, 2fa, etc), because otherwise that project has 100% perfect coverage and always will, even if it's not as ergonomic because it's from OpenAPI.

ljoonal commented 6 months ago

I believe one or more of the fields didn't parse correctly as an OffsetDateTime, but it's been so long since I made these changes prior to submitting this pull request that I don't remember which one(s).

Fair enough, that's why I personally usually start with the integration tests and work from there, since then can just quickly run the test command to check what's failing to parse :smiley:

I'm seemingly the only person using this library right now, likely because the coverage of the API is so low (less than 10%)...

I'd like to think that the README does hint that this crate is meant to compete in quality and not in quantity. I don't really like popularity contents, especially when I'd be on the losing side, so usage is not something I've really estimated/tracked :stuck_out_tongue:

...it's not worth peoples time to create entire requests/responses and then have to validate and test every field to get it submitted.

I'm not trying to discourage from the creation of new API surface to this crate with the PR reviews. The comments are my subjective view of what I'd consider possibly changing after merging a PR before I'd feel like it's polished enough to create a new release.

I leave the comments to facilitate a conversation about the changes the other way too; With the hopes that people contributing changes won't just see me as overwriting their work without a second thought, and that I actually care about the work that they put into the changes. I am sorry if I've come across wrong: You, myself, nor anyone else should have to feel like others think they're entitled to our time or work, as everyone contributing to this crate is spending time doing work for free.

I still appreciate the improvements, and if you so wish, am happy to merge this and make the changes myself that I've commented... When I have more time to do so :smile:

I don't know what your intentions with this library are, but if you intend this to be used outside your own project(s) by the larger community, the coverage needs to grow before the existing vrchatapi-rust project fixes the few problems preventing it from being used (async, 2fa, etc), because otherwise that project has 100% perfect coverage and always will, even if it's not as ergonomic because it's from OpenAPI.

My intentions with this crate is to have something that I feel like would be nicer to use than vrchatapi. This is why I keep being sort of pedantic about things, because leaving them below the level that's acceptable to me would go pretty much directly against that. But I do agree that for it to be useful to more people, it needs to cover more of the API surface. Although I don't think that there's a "before ... fixes the few problems", as the nature of the OpenAPI generation in my mind at least means that it'll never be able to match the goals that I have for this crate. The tradeoff is that I also feel like it'll always beat this crate in terms of API coverage though xP