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 many warnings in components/script
#31717
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few suggestions below. In the future, would it be possible to limit the size of these changes? It's quite difficult to review so many changes all at once.
return { | ||
self.webgl_error(InvalidEnum); | ||
Ok(()) | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels like this should be spit up into two statements instead of putting multiple statements in a block after return
. Please make this change here and other place the same pattern is repeated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return { | |
self.webgl_error(InvalidEnum); | |
Ok(()) | |
}; | |
self.webgl_error(InvalidEnum); | |
return Ok(()); |
components/script/dom/videotrack.rs
Outdated
id: id, | ||
kind: kind, | ||
label: label, | ||
language: language, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this just be the following?
id,
kind,
label,
language,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You resolved the conversation, but you didn't answer the question here. Can these simply be replaced by what I've written above?
components/script
It looks like there is a build failure here. |
When checking the box that says "./mach build -d does not report any errors" please make sure that you've actually tried building this change locally. |
Surely, I have it built locally , let me repeat the process and i will add a screenshot |
Here are the build error from the Linux job (linked below):
|
|
Let me finish the suggested changes and i push them. |
@richarddushime, instead of posting screenshots, please paste console output into a code block like I've done above. Once you make changes, you'll need to run |
Rebuilding Now |
Got the same build error locally after updating main and current branche |
2684c52
to
264c27d
Compare
@richarddushime You don't need to press the "Update branch" button. I only recommend updating against main if there are merge conflicts and in that case it is better to do a manual rebase than to press the button. The reason is that every time you press the button, GitHub will create a merge commit which adds noise to the PR. |
Thanks for this I have just realized the Noise it's causing |
components/script/dom/videotrack.rs
Outdated
id: id, | ||
kind: kind, | ||
label: label, | ||
language: language, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You resolved the conversation, but you didn't answer the question here. Can these simply be replaced by what I've written above?
return { | ||
self.webgl_error(InvalidEnum); | ||
Ok(()) | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return { | |
self.webgl_error(InvalidEnum); | |
Ok(()) | |
}; | |
self.webgl_error(InvalidEnum); | |
return Ok(()); |
return { | ||
self.webgl_error(InvalidOperation); | ||
Ok(()) | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return { | |
self.webgl_error(InvalidOperation); | |
Ok(()) | |
}; | |
self.webgl_error(InvalidOperation); | |
return Ok(()); |
return { | ||
self.webgl_error(InvalidOperation); | ||
Ok(()) | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return { | |
self.webgl_error(InvalidOperation); | |
Ok(()) | |
}; | |
self.webgl_error(InvalidOperation); | |
return Ok(()); |
return { | ||
self.webgl_error(InvalidOperation); | ||
Ok(()) | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return { | |
self.webgl_error(InvalidOperation); | |
Ok(()) | |
}; | |
self.webgl_error(InvalidOperation); | |
return Ok(()); |
🔨 Triggering try run (#8344312267) for Linux WPT |
fa83315
to
27d7e50
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I've pushed a rustfmt
fix for this change. Be sure to run ./mach fmt
before uploading changes.
I m actually building and then running it and I m going to make a final commit I m sorry for delaying it's taking time |
No worries. I've fixed the errors in a new commit. Please don't push any more commits to your branch as that may delay the landing of this change. |
Test results for linux-wpt-layout-2020 from try job (#8344312267): Flaky unexpected result (16)
Stable unexpected results that are known to be intermittent (10)
Stable unexpected results (20)
|
|
Head branch was pushed to by a user without write access
oops just seen this now and I have ready made a commit |
@mrobinson just made a final fix of everything is well now Hopefully |
No worries. I have sent this back to the merge queue. |
./mach build -d
does not report any errors./mach test-tidy
does not report any errors