dart-lang / site-www

Source for Dart website
https://dart.dev
Other
944 stars 683 forks source link

Document todo rule #5401

Open tillulen opened 9 months ago

tillulen commented 9 months ago

Page URL

https://dart.dev/tools/diagnostic-messages.html

Page source

https://github.com/dart-lang/site-www/tree/main/src/tools/diagnostic-messages.md

Describe the problem

The todo rule is briefly mentioned on the Customizing static analysis page, but I couldn’t find any reference documentation for the rule.

Expected fix

I expected to find further documentation about the todo rule either on the Diagnostic messages page or on the Linter rules page.

Additional context

The Customizing static analysis page explains how to disable the todo rule but does not specify what exactly the rule does.

tillulen commented 9 months ago

Are there any other rules that might be missing from the reference documentation?

huycozy commented 9 months ago

Hi @tillulen I can see there is flutter_style_todos being under linter-rules. Can you check and confirm if it's your expected document?

tillulen commented 9 months ago

Hi @huycozy,

Thank you for the quick response!

I can see there is flutter_style_todos being under linter-rules. Can you check and confirm if it's your expected document?

No, that is a different rule. The flutter_style_todos lint seems to enforce a specific format for all TODO comments.

The name of the undocumented rule seems to be just todo. It looks like the todo rule enumerates all the TODO comments the analyzer comes across, so that the developers do not forget about them. I think the rule has the info severity by default, it is not a warning or an error.

Here is the relevant section of the Customizing static analysis page where the todo rule is mentioned:

Ignoring rules

You can ignore specific analyzer diagnostics and linter rules by using the errors: field. List the rule, followed by : ignore. For example, the following analysis options file instructs the analysis tools to ignore the TODO rule:

analyzer:
  errors:
    todo: ignore

Here are two Stack Overflow questions about turning the rule on or off:

  1. dart - TODO comments do not appear in Problems as warning of VS Code after using flutter lint

  2. How to remove todo static analysis for Dart/Flutter projects?

huycozy commented 9 months ago

Thanks for your update, but I still don't know what the expected TODO rules should be added. Do you mean the document should have an example like this SO answer?

analyzer:
  errors:
    todo: info
parlough commented 9 months ago

We don't currently have documentation for every diagnostic, and in those cases the https://github.com/dart-lang/sdk/blob/main/pkg/analyzer/messages.yaml file can be a useful reference.

However I believe todo is a bit special as it is its own type of diagnostic.

@bwilkerson For consistency, would it be possible to surface todo on the page? I'm not sure what that will take since it seems to be set up a bit differently than other diagnostics.

bwilkerson commented 9 months ago

The situation here is somewhat less than ideal.

In my opinion, the TODO diagnostic really shouldn't be a diagnostic at all. We have made it a diagnostic as a concession to the capabilities of the language server's clients (IDEs). It would be better if IDEs asked for TODOs separately, but they don't.

If we were to try to document it, I'm not sure what the documentation would say or how useful it would be. Potentially something like:


todo

{0}

Description

The analyzer produces this diagnostic when it finds a comment in the code that begins with 'TODO', 'FIXME', 'HACK', or 'UNDONE'.

Example

The following code produces this diagnostic because there is a TODO comment:

void f() {
  // TODO(username): Implement this function.
}

Common fixes

Resolve the TODO.


tillulen commented 9 months ago

We don't currently have documentation for every diagnostic, and in those cases the https://github.com/dart-lang/sdk/blob/main/pkg/analyzer/messages.yaml file can be a useful reference.

Thank you for the link! I’ve found a few undocumented diagnostics in messages.yaml, but couldn't find the todo diagnostic there.

huycozy commented 9 months ago

@tillulen Do you find @bwilkerson's explanation appropriate? If you have any more specific suggestions, can you please share them?

tillulen commented 9 months ago

@tillulen Do you find @bwilkerson's explanation appropriate? If you have any more specific suggestions, can you please share them?

Yes, the draft is very useful for me. Most important, its presence in the docs reassures me that todo is indeed a supported diagnostic and we can depend on it to be there in the future. For example, we could turn it into a warning and configure our CI/CD to refuse deploying code that contains unresolved to-dos. If the todo diagnostic is going to be removed in a future version, I would know we can expect an advance deprecation warning and a mention on the What’s New page or in the changelog. Also, I didn't know the diagnostic was triggered by 'FIXME' and the other strings.

Perhaps you could mention the default severity level of todo?

I also find these fixes useful:

Normally you don't recommend ignoring lints – and rightly so. The todo diagnostic, however, can be very noisy, especially in a larger codebase. That may make it harder for developers to go through other lints. For me, it certainly does. When a team has another process in place to track their to-do comments, ignoring the diagnostic will help them keep relevant signal and cut the noise.

bwilkerson commented 9 months ago

Most important, its presence in the docs reassures me that todo is indeed a supported diagnostic and we can depend on it to be there in the future.

The documentation I added to my comment was a hypothetical "this is what it would probably look like if we did add documentation". It doesn't exist anywhere except in the issue comment above.

But yes, the todo diagnostic is likely to continue to be supported, at least until IDEs provide better support for reporting TODO comments to users.

Perhaps you could mention the default severity level of todo?

By default, it has a severity of "INFO".

If you mean mention it in the docs, we aren't currently documenting the default severity level of any diagnostics, but that's definitely something we could consider.

The todo diagnostic, however, can be very noisy, especially in a larger codebase.

True. Both IntelliJ and VS Code have support for removing TODO comments from the display.

I also find these fixes useful ...

Those are both reasonable, but I'll point out that they would circumvent your idea of using TODO comments as a signal in your CI/CD. At least the first leaves you with some signal (in the form of an issue); the second hides the problem completely, which might not be what you want.

tillulen commented 9 months ago

For future reference, todo is implemented in these files:

tillulen commented 9 months ago

The documentation I added to my comment was a hypothetical "this is what it would probably look like if we did add documentation". It doesn't exist anywhere except in the issue comment above.

Yes. Sorry, my wording was not precise. I meant “if todo was documented, that would reassure me...” etc. That was meant to be a case for documenting the diagnostic rather than leaving it undocumented.

If you mean mention it in the docs, we aren't currently documenting the default severity level of any diagnostics, but that's definitely something we could consider.

Yes, I thought mentioning the severity in the docs could be useful.

True. Both IntelliJ and VS Code have support for removing TODO comments from the display.

Oh, that’s a helpful feature! I didn’t notice the filter in VS Code. Thank you for mentioning it.

(For reference, to filter out all lints that match the string TODO, use a !TODO filter in the upper right corner of the Problems panel. I couldn’t quickly find a way to filter out two different strings at the same time, for example if I wanted to hide lints that match either TODO or HACK.)

dam-ease commented 9 months ago

Thanks for your response @tillulen. Per your last https://github.com/dart-lang/site-www/issues/5401#issuecomment-1858766799, do you mean the explanations are quite clear or is there any other thing you would like to point out for action or it's safe to close this?

tillulen commented 9 months ago

Thanks for your response @tillulen. Per your last #5401 (comment), do you mean the explanations are quite clear or is there any other thing you would like to point out for action or it's safe to close this?

I think there might be a misunderstanding. I’ve received the answers to my questions and I appreciate Brian taking the time to clarify things for me.

This issue, however, focuses on incorporating those answers into the documentation and making them easily accessible to everyone.

If the team decides no documentation updates are planned, closing this issue as Won’t Fix would clearly communicate that decision. Otherwise, please leave it open.

parlough commented 9 months ago

I think while the documentation/common fixes portion for the todo rule isn't super helpful, having an entry is still helpful in terms of identifying which diagnostics you can configure/ignore. So I'll keep the issue open to track surfacing that improvement. Thanks!

bwilkerson commented 9 months ago

If the docs have value, then I'm fine with adding them.

There isn't any entry in messages.yaml for these codes, so without bigger changes we could update the generator to add these docs manually. It's kind of ugly, but unless someone has time to cause the TODO codes to be generated I don't see an alternative.

parlough commented 9 months ago

Sounds good @bwilkerson, thanks for the context and suggestion. No need to prioritize this on your team's end.

When I have a chance, I'll take a look, or someone else who would like to see the issue fixed :)