Skip to content

Some clean-up refactorings for UsedAssemblyReferences feature:#41268

Merged
AlekseyTs merged 2 commits intodotnet:features/UsedAssemblyReferencesfrom
AlekseyTs:UsedAssemblies_14
Jan 29, 2020
Merged

Some clean-up refactorings for UsedAssemblyReferences feature:#41268
AlekseyTs merged 2 commits intodotnet:features/UsedAssemblyReferencesfrom
AlekseyTs:UsedAssemblies_14

Conversation

@AlekseyTs
Copy link
Copy Markdown
Contributor

  • Update license comments
  • Refactorings to address previous PR feedback and prototype comments

- Update license comments
- Refactorings to address previous PR feedback and prototype comments
@AlekseyTs AlekseyTs requested a review from a team as a code owner January 28, 2020 20:43
@jcouv jcouv self-assigned this Jan 28, 2020
internal CSDiagnosticInfo Add(ErrorCode code, Location location)
{
var info = new CSDiagnosticInfo(code);
DiagnosticBag?.Add(new CSDiagnostic(info, location));
Copy link
Copy Markdown
Contributor

@cston cston Jan 28, 2020

Choose a reason for hiding this comment

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

DiagnosticBag?.Add(new CSDiagnostic(info, location)) [](start = 12, length = 52)

Consider replacing with Add(info, location) here and in the two methods below. #Resolved


Friend Overloads Function Add(code As ERRID, location As Location) As DiagnosticInfo
Dim info = ErrorFactory.ErrorInfo(code)
DiagnosticBag?.Add(New VBDiagnostic(info, location))
Copy link
Copy Markdown
Contributor

@cston cston Jan 28, 2020

Choose a reason for hiding this comment

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

DiagnosticBag?.Add(New VBDiagnostic(info, location)) [](start = 12, length = 52)

Consider using Add(info, location) here and method below. #Resolved

{
Diagnostics = diagnostics.NullToEmpty();
Dependencies = dependencies.NullToEmpty();
_diagnostics = diagnostics.NullToEmpty();
Copy link
Copy Markdown
Member

@jcouv jcouv Jan 28, 2020

Choose a reason for hiding this comment

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

.NullToEmpty() [](start = 38, length = 14)

nit: Do we need the .NullToEmpty() here, since already done when returning from properties above? #Resolved

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Do we need the .NullToEmpty() here, since already done when returning from properties above?

I don't think it hurts.


In reply to: 372085003 [](ancestors = 372085003)

Copy link
Copy Markdown
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM Thanks (iteration 1)

@AlekseyTs AlekseyTs merged commit bc6b266 into dotnet:features/UsedAssemblyReferences Jan 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants