Skip to content

Remove misleading default value for TodoCommentOptions.TokenList#52144

Merged
sharwell merged 3 commits intodotnet:mainfrom
sharwell:comment-tokens
Apr 1, 2021
Merged

Remove misleading default value for TodoCommentOptions.TokenList#52144
sharwell merged 3 commits intodotnet:mainfrom
sharwell:comment-tokens

Conversation

@sharwell
Copy link
Contributor

This change removes the default value from TodoCommentOptions.TokenList. The previous default value was problematic in two ways:

  1. When the default value was used, the solution crawler would compute a bunch of data for documents which would be discarded later when the true value was loaded
  2. The default value allowed bugs like this to be hidden for a long time, since it was difficult to tell when the correct value was used compared to a default that was "close" to correct (for some users)

A fast path was added to reduce the amount of work performed by the solution crawler if it runs with a defaulted option.

⚠️ This change is going to make it very obvious that we have a severe bug in OOP options synchronization. In 100% of cases I tested, TODO comments are using the default value and not the value set by users.

@sharwell sharwell requested a review from a team as a code owner March 25, 2021 16:52
@ghost ghost added the Area-IDE label Mar 25, 2021
@tmat
Copy link
Member

tmat commented Mar 25, 2021

@CyrusNajmabadi

The correct value for this option is sourced from ITaskList. Including a
default value hides cases where options fail to load.
@sharwell sharwell marked this pull request as draft March 26, 2021 15:15
@sharwell
Copy link
Contributor Author

@CyrusNajmabadi planning to merge this after we figure out how to make it work properly so we don't wreak the world 😄

@CyrusNajmabadi
Copy link
Contributor

Can we merge it asking with a fix to the underlying issue? Do we know why the option here isn't serializing properly to oop?

@sharwell
Copy link
Contributor Author

sharwell commented Apr 1, 2021

This is ready to merge once #52335 merges.

@sharwell sharwell merged commit f5bd51a into dotnet:main Apr 1, 2021
@ghost ghost added this to the Next milestone Apr 1, 2021
@sharwell sharwell deleted the comment-tokens branch April 1, 2021 19:42
@dibarbet dibarbet modified the milestones: Next, 16.10.P3 Apr 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants