Skip to content

Fix a couple of cases where feature options were not being hooked up.#66241

Merged
CyrusNajmabadi merged 3 commits intodotnet:mainfrom
CyrusNajmabadi:fixOptions
Jan 5, 2023
Merged

Fix a couple of cases where feature options were not being hooked up.#66241
CyrusNajmabadi merged 3 commits intodotnet:mainfrom
CyrusNajmabadi:fixOptions

Conversation

@CyrusNajmabadi
Copy link
Copy Markdown
Contributor

No description provided.

@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner January 4, 2023 20:20
@ghost ghost added the Area-IDE label Jan 4, 2023
Copy link
Copy Markdown
Member

@genlu genlu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we add a test for this?

@CyrusNajmabadi
Copy link
Copy Markdown
Contributor Author

possibly. @tmat for ideas. Generally speaking though, i'm not a fan of these option types necessarily having optional values. If we just made them required it would ensure that they had to be properly updated when things changed.

@tmat
Copy link
Copy Markdown
Member

tmat commented Jan 4, 2023

We have a test that validates this, it's just missing this particular option type: TestGlobalOptions.ReadingOptionsFromGlobalOptions.

@CyrusNajmabadi Could you add the options to the test, run w/o your change and see if it fails?

If we just made them required it would ensure that they had to be properly updated when things changed.

Unfortunately, that would also mean that you'd need to specify all the options every time. Some of these types have dozens of options, so that's very inconvenient to use if you just need to change a couple of options. Some of these types are only ever initialized from global options, so perhaps they can have all the properties required. But the plan is to make some of these option types public (to replace OptionSet, for example in Formatter, Simplifier, and other APIs) and we definitely don't want to require all the properties specified in that case.

@CyrusNajmabadi
Copy link
Copy Markdown
Contributor Author

@tmat Yup, that's tough. I don't have a godo answer here. I'll update the test though!

@tmat
Copy link
Copy Markdown
Member

tmat commented Jan 4, 2023

The test is the best solution I could find.

In principle, we want to mark the call-site as requiring all arguments. A special comment and analyzer would be an option...

@CyrusNajmabadi CyrusNajmabadi merged commit dba0af9 into dotnet:main Jan 5, 2023
@ghost ghost added this to the Next milestone Jan 5, 2023
CyrusNajmabadi added a commit to CyrusNajmabadi/roslyn that referenced this pull request Jan 5, 2023
CyrusNajmabadi added a commit to CyrusNajmabadi/roslyn that referenced this pull request Jan 5, 2023
CyrusNajmabadi added a commit to CyrusNajmabadi/roslyn that referenced this pull request Jan 5, 2023
arunchndr pushed a commit that referenced this pull request Jan 5, 2023
@CyrusNajmabadi CyrusNajmabadi deleted the fixOptions branch January 7, 2023 00:52
@Cosifne Cosifne modified the milestones: Next, 17.6 P1 Jan 31, 2023
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