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

Breaking API Change #4170

Closed
tannergooding opened this issue Jul 28, 2015 · 6 comments
Closed

Breaking API Change #4170

tannergooding opened this issue Jul 28, 2015 · 6 comments
Labels
Area-Compilers Concept-API This issue involves adding, removing, clarification, or modification of an API. Resolution-Fixed The bug has been fixed and/or the requested behavior has been implemented
Milestone

Comments

@tannergooding
Copy link
Member

PR #4169 Makes a breaking change to the Public API as part of the Toolset Update. Specifically it removes the following from src\Compiles\VisualBasic\Portable\PublicAPI.txt:

  • Microsoft.CodeAnalysis.VisualBasic.VisualBasicCommandLineParser.New(isInteractive As Boolean = False) -> Void
  • Overrides Microsoft.CodeAnalysis.VisualBasic.VisualBasicCommandLineParser.RegularFileExtension() -> String
  • Overrides Microsoft.CodeAnalysis.VisualBasic.VisualBasicCommandLineParser.ScriptFileExtension() -> String

A decision will need to be made on whether or not we need to keep this breaking change or if we should take on the public API increases that will be required to revert it.

@tannergooding tannergooding added Concept-API This issue involves adding, removing, clarification, or modification of an API. Area-Compilers labels Jul 28, 2015
@tannergooding
Copy link
Member Author

FYI. @jaredpar, @AnthonyDGreen

@davkean
Copy link
Member

davkean commented Jul 28, 2015

Removal of overrides shouldn't be a breaking change (and we're shouldn't log them in PublicAPI.txt). Removal of the constructor is a breaking change.

@sharwell
Copy link
Member

Removal of overrides shouldn't be a breaking change (and we're shouldn't log them in PublicAPI.txt).

It is a breaking change if the override has default argument values which differ from those in the base definition. This doesn't apply to the case above but it's something to be aware of before suppressing overrides from the public API.

@jaredpar
Copy link
Member

@davkean @sharwell

Yes it's not new public API but it is a part of the overall public API surface. The analyzer defaulted to extra verbosity here. Removal of this alone wouldn't constitute a breaking change.

@davkean
Copy link
Member

davkean commented Jul 28, 2015

It is a breaking change if the override has default argument values which differ from those in the base definition. This doesn't apply to the case above but it's something to be aware of before suppressing overrides from the public API.

Technically yes. Would we ever do this - no. And it would result in different behavior between C# and VB (C# calls the base method, VB calls the override).

@gafter
Copy link
Member

gafter commented Jul 31, 2015

Fixed in #4186 (documented the breaking change)

@gafter gafter closed this as completed Jul 31, 2015
@gafter gafter added the Resolution-Fixed The bug has been fixed and/or the requested behavior has been implemented label Jul 31, 2015
@gafter gafter added this to the 1.1 milestone Jul 31, 2015
@gafter gafter removed their assignment Jul 31, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers Concept-API This issue involves adding, removing, clarification, or modification of an API. Resolution-Fixed The bug has been fixed and/or the requested behavior has been implemented
Projects
None yet
Development

No branches or pull requests

5 participants