-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
misc: replace TODOs with github issue links #7049
Conversation
@@ -129,8 +129,6 @@ class OptimizedImages extends Gatherer { | |||
* @return {Promise<?{fromProtocol: boolean, originalSize: number, jpegSize: number, webpSize: number}>} | |||
*/ | |||
calculateImageStats(driver, networkRecord) { | |||
// TODO(phulce): remove this dance of trying _getEncodedResponse with a fallback when Audits |
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.
at this point, I'm not sure it makes sense. It's a solid fallback option for when the protocol fails.
@@ -51,7 +51,6 @@ const RESOURCE_TYPES = { | |||
module.exports = class NetworkRequest { | |||
constructor() { | |||
this.requestId = ''; | |||
// TODO(phulce): remove default DevTools connectionId |
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.
I'm not convinced this is necessary anymore
@@ -28,7 +28,6 @@ trap teardown EXIT | |||
|
|||
colorText "Generating a fresh LHR..." "$purple" | |||
set -x | |||
# TODO(phulce): add a lantern LHR-differ |
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.
I'm not convinced this is necessary anymore
@@ -231,7 +231,6 @@ class NetworkRecorder extends EventEmitter { | |||
const modifiedData = { | |||
...data, | |||
// Copy over the initiator as well to match DevTools behavior | |||
// TODO(phulce): abandon this DT hack and update Lantern graph to handle it |
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.
I'm not convinced this is necessary anymore, it's probably easier to just stay in sync with wht devtools does
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.
wfm.
@brendankenny does this work out with your papercut plans?
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.
- For complicated things, I'd favor not having a TODO in the first place.
- For simple things, the link indirection makes it difficult to quickly evaluate how big a deal the TODO is (like, is the code fundamentally wrong but good enough for now? Or is it just missing some unimportant corner case or does it just need some sprucing up).
And, really, for simple things, a comment and a real issue probably better handles the above.
So...it's not clear to me how much the code changes improve things, but issues tracking the problems is better. And the full link is way better than just adding a #7045
to a comment, which only works in the github UI. So I think this is fine.
lighthouse-core/gather/driver.js
Outdated
@@ -1241,7 +1241,7 @@ class Driver { | |||
* @return {Promise<void>} | |||
*/ | |||
async beginEmulation(settings) { | |||
// TODO(phulce): remove this flag on next breaking change | |||
// TODO: https://github.com/GoogleChrome/lighthouse/issues/7044 |
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.
I'm not sure if it's worth keeping the TODO for things like this that aren't so targeted at a single point (or that single point is easy to find again since the goal is to remove something altogether)
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.
removed
sooooooo it sounds like you'd prefer just removing all of them? I'm down for that. |
ha, probably. I'm also fine with this PR for now. What do you want to do? |
I'll remove most of them for now. There are one or two that I like having the easy exact line handle for finding when we actually address the issue, but most are stand alone. |
Summary
Cleans up my TODOs with github issues so they're more visible and easier to prioritize.