TODO Audits - 1 #53604
TODO Audits - 1 #53604
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat. Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
fluttergithubbot
added
the
team
There was a problem hiding this comment.
LGTM for the TODO's with my name.
Three of @dnfield's TODO's are removed without code changes. Is that expected?
dev/ci/docker_linux/Dockerfile
Outdated
| @@ -96,10 +96,6 @@ RUN dpkg-query -L nodejs | |||
| # Install Firebase | |||
| # This is why we need nodejs installed. | |||
| RUN /usr/bin/npm --verbose install -g firebase-tools | |||
| # TODO(dnfield): Remove this once Firebase has a fix upstream for | |||
There was a problem hiding this comment.
I'm not sure if we should remove this (the linked issue was closed because uploads were stable WITH this patch), but if we should, we should delete the patch_firebase.sh script too. @dnfield
There was a problem hiding this comment.
Firebase was going to patch this in, this was a manual hack until they did.
There was a problem hiding this comment.
We've also mitigated this by just not uploading as much.
There was a problem hiding this comment.
Let's remove this and the patch. If docs start timing out we can add it back, but I'm pretty sure we're covered now. The underlying change is that we used to embed a git hash in every file in the docs, and now it's just in one file that's dynamically loaded by all the others.
There was a problem hiding this comment.
(maybe in a separate change, in case it does cause problems it's more easily reverted)
I've left notes for each. |
| @@ -19,7 +19,7 @@ class Stock { | |||
| Stock(this.symbol, this.name, this.lastSale, this.marketCap, this.percentChange); | |||
|
|
|||
| Stock.fromFields(List<String> fields) { | |||
| // FIXME: This class should only have static data, not lastSale, etc. | |||
| // TODO(jackson): This class should only have static data, not lastSale, etc. | |||
There was a problem hiding this comment.
Does this really require a TODO? This code is really only used for benchmarking at this point. Maybe just leave the comment about how it would be a better design, but it's being left this way for simplicity?
/cc @collinjackson
There was a problem hiding this comment.
Sure thing if everyone is on board. :) There are a lot of TODOs like that.
dev/devicelab/manifest.yaml
Outdated
| codegen_integration_win: | ||
| description: > | ||
| Runs codegeneration and verifies that it can execute | ||
| correctly. | ||
| stage: devicelab_win | ||
| required_agent_capabilities: ["windows/android"] |
There was a problem hiding this comment.
nit: might be good to do this in a separate change so that it can be independently reverted more easily.
There was a problem hiding this comment.
Maybe same for the docker change. I'm not familiar with the original docs issue, but I'm guessing it was a flake?
|
Thanks for the feedback, I'll update this and make the splits into separate PRs where requested. :) |
|
All cleaned up. |
Description
This is the first of several TODO related PRs. I am auditing the codebase for TODOs that are associated with closed issues, or are no longer relevant.
If any of these TODOs are still accurate, they will need a new issue attributed to them.
Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]). This will ensure a smooth and quick review process.///).flutter analyze --flutter-repo) does not report any problems on my PR.Breaking Change
Did any tests fail when you ran them? Please read Handling breaking changes.