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

clippy: Fix some warnings in components/script #31735

Merged
merged 7 commits into from Mar 19, 2024
Merged

Conversation

six-shot
Copy link
Contributor

@six-shot six-shot commented Mar 18, 2024


  • There are tests for these changes OR
  • These changes do not require tests because ___

Comment on lines 67 to 68
impl Default for Identities {
fn default() -> Self {
Copy link
Member

Choose a reason for hiding this comment

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

You also need to update all references to Identities::new() to be Identities::default() if you change the name of the method.

@mrobinson
Copy link
Member

Does this build locally? Before putting the "x" in the box that says " ./mach build -d does not report any errors" please actually run ./mach build locally.

@six-shot
Copy link
Contributor Author

I apologize sir I thought I did run the build for this PR I’ll fix it right away

@six-shot
Copy link
Contributor Author

@mrobinson Please review this again

@six-shot six-shot requested a review from mrobinson March 18, 2024 21:29
@@ -114,8 +114,8 @@ impl MediaFragmentParser {
}
} else {
let mut iterator = fragment.split(',');
let start = parse_hms(iterator.next().ok_or_else(|| ())?)?;
let end = parse_hms(iterator.next().ok_or_else(|| ())?)?;
let start = parse_hms(iterator.next().ok_or(()))?;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the build error you are having originates here, since it expects an &str but it is receiving a Result. One way to solve it would be adding back the second ?, as it will unwrap the Result type. You can probably also get rid of the ok_or(()) operator since ? should also work for Option types and we are not using the error type. This goes for lines 117, 118, 317, 318, 322 and 333.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I fixed all 6 of them already… I’m trying to build it before pushing to be sure there no more errors Thanks @eerii

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The PR is ready to go now

Copy link
Contributor

Choose a reason for hiding this comment

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

A member from the servo team will have to approve the pr but all the changes are looking good to me, thanks!

@six-shot six-shot requested a review from eerii March 19, 2024 01:04
@mrobinson mrobinson changed the title fix clippy problems clippy: Fix some warnings in components/script. Mar 19, 2024
@mrobinson mrobinson changed the title clippy: Fix some warnings in components/script. clippy: Fix some warnings in components/script Mar 19, 2024
@mrobinson mrobinson added this pull request to the merge queue Mar 19, 2024
Merged via the queue into servo:main with commit 06a021d Mar 19, 2024
15 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.

None yet

3 participants