Skip to content
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

Document todo rule #5401

Open
tillulen opened this issue Dec 11, 2023 · 17 comments
Open

Document todo rule #5401

tillulen opened this issue Dec 11, 2023 · 17 comments
Labels
d.enhancement Improves docs with specific ask e2-days Can complete in < 5 days of normal, not dedicated, work from.page-issue Reported in a reader-filed concern p3-low Valid but not urgent concern. Resolve when possible. Encourage upvote to surface. st.triage.ltw Indicates Lead Tech Writer has triaged t.diagnostics Relates to diagnostics, analysis, or linting of code

Comments

@tillulen
Copy link

tillulen commented Dec 11, 2023

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 tillulen added the from.page-issue Reported in a reader-filed concern label Dec 11, 2023
@tillulen
Copy link
Author

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

@huycozy huycozy added the st.triage.triage-team Triage team reviewing and categorizing the issue label Dec 12, 2023
@huycozy
Copy link
Member

huycozy commented Dec 12, 2023

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?

@huycozy huycozy added the act.wait-for-customer Needs response from customer label Dec 12, 2023
@tillulen
Copy link
Author

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?

@github-actions github-actions bot removed the act.wait-for-customer Needs response from customer label Dec 12, 2023
@huycozy
Copy link
Member

huycozy commented Dec 14, 2023

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

@huycozy huycozy added the act.wait-for-customer Needs response from customer label Dec 14, 2023
@parlough
Copy link
Member

parlough commented Dec 14, 2023

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
Copy link
Member

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
Copy link
Author

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.

@github-actions github-actions bot removed the act.wait-for-customer Needs response from customer label Dec 15, 2023
@huycozy
Copy link
Member

huycozy commented Dec 15, 2023

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

@huycozy huycozy added the act.wait-for-customer Needs response from customer label Dec 15, 2023
@tillulen
Copy link
Author

tillulen commented Dec 15, 2023

@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.

@github-actions github-actions bot removed the act.wait-for-customer Needs response from customer label Dec 15, 2023
@bwilkerson
Copy link
Member

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
Copy link
Author

For future reference, todo is implemented in these files:

@tillulen
Copy link
Author

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
Copy link

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?

@dam-ease dam-ease added the act.wait-for-customer Needs response from customer label Dec 21, 2023
@tillulen
Copy link
Author

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.

@github-actions github-actions bot removed the act.wait-for-customer Needs response from customer label Dec 21, 2023
@parlough
Copy link
Member

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!

@parlough parlough added d.enhancement Improves docs with specific ask e2-days Can complete in < 5 days of normal, not dedicated, work t.diagnostics Relates to diagnostics, analysis, or linting of code p3-low Valid but not urgent concern. Resolve when possible. Encourage upvote to surface. and removed st.triage.triage-team Triage team reviewing and categorizing the issue labels Dec 21, 2023
@bwilkerson
Copy link
Member

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
Copy link
Member

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 :)

@atsansone atsansone added the st.triage.ltw Indicates Lead Tech Writer has triaged label Feb 13, 2024
@atsansone atsansone changed the title todo rule is undocumented Document todo rule Feb 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
d.enhancement Improves docs with specific ask e2-days Can complete in < 5 days of normal, not dedicated, work from.page-issue Reported in a reader-filed concern p3-low Valid but not urgent concern. Resolve when possible. Encourage upvote to surface. st.triage.ltw Indicates Lead Tech Writer has triaged t.diagnostics Relates to diagnostics, analysis, or linting of code
Projects
None yet
Development

No branches or pull requests

6 participants