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

CI: Enable clippy #101

Merged
merged 2 commits into from
Jan 9, 2022
Merged

CI: Enable clippy #101

merged 2 commits into from
Jan 9, 2022

Conversation

alexkirsz
Copy link
Contributor

This PR enables clippy and applies a few fixes to make it lint successfully. See additional comments below.

override: true
- uses: Swatinem/rust-cache@v1
- run: sudo apt-get update
- run: sudo apt install libwebkit2gtk-4.0-dev libappindicator3-dev libgtk-3-dev libappindicator3-dev
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Clippy would eventually fail when checking the code looking for the 'pango' package, so I added all the dependencies we need for compiling on ubuntu-latest.

Copy link
Member

Choose a reason for hiding this comment

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

Cool, thank you for doing that. I accidentally added libappindicator3 twice, so maybe we want to remove that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@@ -745,6 +745,7 @@ impl ScopeState {
/// use_hook(|| Rc::new(RefCell::new(initial_value())))
/// }
/// ```
#[allow(clippy::mut_from_ref)]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not qualified to determine whether the mut_from_ref usage here is sound so I allowed it.

Copy link
Member

Choose a reason for hiding this comment

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

There's a bunch of safety dynamics to enable borrowed lifetimes through the tree, and this is the consequence :/

@@ -1,3 +1,4 @@
#![allow(warnings)]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since this package is very much WIP, I didn't see the point in allowing or fixing each instance individually. We'll need to remove this once the package is closer to ready.

@@ -38,6 +38,7 @@ impl SsrRenderer {
}
}

#[allow(clippy::needless_lifetimes)]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe the below could be

pub fn render_lazy(f: LazyNodes<'_, '_>) -> String {

But since we name the lifetimes in the Safety comment below, I allowed it.

@alexkirsz alexkirsz requested a review from jkelleyrtp January 8, 2022 15:36
override: true
- uses: Swatinem/rust-cache@v1
- run: sudo apt-get update
- run: sudo apt install libwebkit2gtk-4.0-dev libappindicator3-dev libgtk-3-dev libappindicator3-dev
Copy link
Member

Choose a reason for hiding this comment

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

Cool, thank you for doing that. I accidentally added libappindicator3 twice, so maybe we want to remove that.

@@ -745,6 +745,7 @@ impl ScopeState {
/// use_hook(|| Rc::new(RefCell::new(initial_value())))
/// }
/// ```
#[allow(clippy::mut_from_ref)]
Copy link
Member

Choose a reason for hiding this comment

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

There's a bunch of safety dynamics to enable borrowed lifetimes through the tree, and this is the consequence :/

@jkelleyrtp jkelleyrtp merged commit 29bf424 into DioxusLabs:master Jan 9, 2022
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