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

Follow up on public api review notes #43435

Closed
333fred opened this issue Apr 17, 2020 · 7 comments · Fixed by #43527
Closed

Follow up on public api review notes #43435

333fred opened this issue Apr 17, 2020 · 7 comments · Fixed by #43527
Assignees
Labels
Area-IDE Concept-Design Debt Engineering Debt, Design Debt, or poor product code quality
Milestone

Comments

@333fred
Copy link
Member

333fred commented Apr 17, 2020

  • Project.RemoveDocuments: Everything else in this type uses IEnumerable<T>, not ImmutableArray<T>. If we could start again from day 1 we might make everything that, but given that everything today uses the former, we should do so here. This applies to the overloads on Solution as well.
  • Solution.RemoveAdditonalDocuments/RemoveAnalyzerConfigDocuments: We added these to Solution, but not to project, even though that also has the invididual Remove methods. We should add them to Project as well.
@333fred 333fred added this to the 16.6 milestone Apr 17, 2020
@jinujoseph jinujoseph modified the milestones: 16.6, 16.7 Apr 17, 2020
@jinujoseph jinujoseph added the Concept-Design Debt Engineering Debt, Design Debt, or poor product code quality label Apr 17, 2020
@jinujoseph jinujoseph modified the milestones: 16.7, 16.6 Apr 17, 2020
@jasonmalinowski
Copy link
Member

Project.RemoveDocuments: Everything else in this type uses IEnumerable, not ImmutableArray. If we could start again from day 1 we might make everything that, but given that everything today uses the former, we should do so here. This applies to the overloads on Solution as well.

So @333fred and @CyrusNajmabadi, it looks like we're already a bit inconsistent here. Solution.AddDocuments (which is already a shipped API) is already taking an ImmutableArray, so when we added RemoveDocuments we were being consistent with that. I can switch the Remove methods to IEnumerable, but then that just results in a different inconsistency. Is that something we feel is necessary here?

@jasonmalinowski
Copy link
Member

Also as a reminder any change to a signature here induces a fairly high test cost given we need to take any chance here through VS shiproom.

@333fred
Copy link
Member Author

333fred commented Apr 21, 2020

I think I'd prefer consistency with the symmetric method (AddDocuments) than with the rest of the methods in Solution (which may or may not be related). It would have been good if they were all ImmutableArray to start with, but we are where we are.

@jasonmalinowski
Copy link
Member

@333fred: so in that case, the only item to do then is adding the Project.Remove* methods that match the Solution.Remove* methods? OK if we do that in 16.7 so as to avoid QB process for this?

Tagging @tmat too on the IEnumerable vs. ImmutableArray since he was messing with some of that recently too.

@333fred
Copy link
Member Author

333fred commented Apr 21, 2020

I think that's fine.

@jasonmalinowski
Copy link
Member

@tmat or @CyrusNajmabadi any concern about fixing just the missing methods, i.e. the second bullet point and not the first?

@jasonmalinowski
Copy link
Member

Given it's now too late to fix anyways, going with the PR that only fixes the second issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-IDE Concept-Design Debt Engineering Debt, Design Debt, or poor product code quality
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants