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

Extract method cleanup #76550

Open
wants to merge 54 commits into
base: main
Choose a base branch
from

Conversation

CyrusNajmabadi
Copy link
Member

Cleanup work in the extract method codebase before doign some more major feature work. I've documented int eh PR what changes i'm making. Should be reviewed with whitespace off.

@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner December 22, 2024 04:03
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Dec 22, 2024

public bool IsExtractMethodOnSingleStatement()
Copy link
Member Author

Choose a reason for hiding this comment

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

moved to SelectionInfo.GetSelectionType. This should be computed once, not over and oer again.

{
return false;
}
if (!this.ContainsValidSelection)
Copy link
Member Author

Choose a reason for hiding this comment

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

pulled out common logic from VB and C#.

static bool CanMergeExistingSpineWithCurrent(ISyntaxFacts syntaxFacts, T existing, T current)
=> syntaxFacts.AreStatementsInSameContainer(existing, current);
}
protected (TStatementSyntax firstStatement, TStatementSyntax lastStatement)? GetStatementRangeContainingSpan(
Copy link
Member Author

Choose a reason for hiding this comment

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

no need for this method to be generic, it was always being passed the TStatementSyntax type.

}

return (firstStatement, lastStatement);

static bool CanMergeExistingSpineWithCurrent(ISyntaxFacts syntaxFacts, T existing, T current)
Copy link
Member Author

Choose a reason for hiding this comment

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

inlined this method. it no longer uses syntaxFacts, it ust calls into an abstract method on this type.

return (firstStatement, lastStatement);
}

protected sealed class SelectionInfo
Copy link
Member Author

Choose a reason for hiding this comment

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

extracted to file.

if (token1.Parent == null || token2.Parent == null)
{
return null;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

these can't happen in real code. so just changed to GetrequiredParent() to ensure that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead VSCode
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant