phetsims / tasks

General tasks for PhET that will be tracked on Github
MIT License
5 stars 2 forks source link

One issue to hold every TODO in the project #1129

Open zepumph opened 1 year ago

zepumph commented 1 year ago

From https://github.com/phetsims/tasks/issues/1128, it will be a good starting point if every TODO that doesn't currently have an issue assigned to it, points to this issue instead. Then we will get the benefits of having closed issues reopened when TODOs point there for all repos.

zepumph commented 1 year ago

There are currently 890 TODOs pointing to this issue. Fun!

pixelzoom commented 1 year ago

We discussed this issue in dev meeting. Our previous decision to require TODOs for issues in common-code and published sims is not being followed and is not supported by all devs. And I don't know what the consensus was -- it seems like it's up to each dev, and I think that's problematic.

We did agree that no new references to this issue should be created.

I went a step further, and did some work to eliminate references to this issue. Here's a summary:

There were a number of places where a link to this issue was errorneously added, because the issue was not on the same line as "TODO". In those cases, I simply removed the reference to those issues. For example in MagnetRegionManager:

    // TODO: The code below looks wrong.  The return type doesn't match the header docs, and the whole thing is too https://github.com/phetsims/tasks/issues/1129
    //  tightly coupled to the way bounds are managed in the model.  See
    // https://github.com/phetsims/faradays-law/issues/164. Also, it seems like it should return null instead of -1 if
    // no intersection is found.

In these repos, there were a small number of TODOs, so I created specific GitHub issues:

In these repos, I created one repo-specific "Address TODOs" issue:

Sim repos that continue to point to this issue:

Common-code repos that continue to point to this issue. dot, kite, and scenery are the obvious problems here.

pixelzoom commented 1 year ago

Back to @zepumph to continue work on this issue.

Regarding the errorneous TODOs identified by the lint rule... In Slack#DM, @zepumph said:

Yes I think we could adapt our rule to support that. Could you make an issue in chipper please? It is because eslint thinks of a single "comment" only as one line comment or one block comment. It doesn't combine those. I hadn't seen that case before but obviously we want to support that (I certainly write TODOs like that).

zepumph commented 1 year ago

After the above work, there are 189 TODOs pointing to this issue. That is better than 890! @pixelzoom did a good job of outlining where they live right now. I'm going to unassign for now. Thanks all.