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

More refurbishment #4

Merged
merged 7 commits into from
May 27, 2022
Merged

Conversation

woodruffw
Copy link
Contributor

This is my sequel to #3 🙂

Key changes:

  • Made most of the modules and structure fields pub by default, preserving interior consistency by marking the structs with #[readonly::make]
  • Removed the mandatory documentation enforcement (since it was producing hundreds of redundant errors)
  • Rewrite some of the uses to reduce duplication
  • Commented out unused fields in the intermediate serde structs (since serde will simply skip unknown fields)

Let me know what you think (especially if you think I went a little too overboard on chasing zero warnings).

src/client.rs Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
@xeals
Copy link
Owner

xeals commented May 20, 2022

Most looks pretty reasonable. I probably had some idea in mind RE: visibility of collections and the functions that are otherwise unused at the moment, but I have no idea what it could have been, so happy to expose those.

Commented out unused fields in the intermediate serde structs (since serde will simply skip unknown fields)

Leaving them commented for documentation is good.

@woodruffw
Copy link
Contributor Author

Should be good for another pass. I've also run cargo fix --edition to modernize the library to Rust 2021.

@xeals
Copy link
Owner

xeals commented May 27, 2022

LGTM. Thanks again!

@xeals xeals merged commit f19dbc8 into xeals:master May 27, 2022
@woodruffw woodruffw deleted the ww/more-refurbish branch May 27, 2022 02:47
@woodruffw
Copy link
Contributor Author

No problem! Thanks for the fast review and merge. I'll do a third (and final) round for simplifying some of the deserialization traits, and then I'll finally stop 🙂

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