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

Add rustfmt.toml configuration and re-format everything + fix basic warnings #279

Merged
merged 2 commits into from
Jan 1, 2024

Conversation

daniel-abramov
Copy link
Contributor

So that contributions from multiple people are more consistent w.r.t. code style and formatting 🙂

Note, that this one does not include all of the cargo clippy fixes since there are quite a few of them that require manual intervention. I did not want to make them part of this PR (since unlike the rest of the changes these would not be automated, hence would require a more rigorous review of whether the user-written code changes anything).

Also, cargo check still produces 3 warnings that I did not fix as they would need some minor changes:

  • RemoteTrackPublication::on_subscription_status_changed and RemoteTrackPublication::on_permission_status_changed are pub(crate) and never used within a crate. I was not sure if I'm allowed to remove them (since we don't use them) or if I need to make them pub (that would require making the rest of the things pub as well, for consistency).
  • RtcSession::publisher() is pub, but never used (not sure why it complains about it given that it is pub, perhaps it's never exported or something).

Also I added a tiny simple job to check the formatting so that we can keep it consistent.

Copy link
Member

@theomonnom theomonnom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm, TIL about use_small_heuristics

@theomonnom
Copy link
Member

Thanks 🙏
I’ll try to address the other warnings

@theomonnom theomonnom merged commit 3ea84b1 into livekit:main Jan 1, 2024
13 of 16 checks passed
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