Skip to content

[SR-5744] Refactoring action to convert if-let to guard-let and vice versa#24566

Merged
nkcsgexi merged 2 commits into
swiftlang:masterfrom
Regno:feature/vlasov/SR-5744
Jul 11, 2019
Merged

[SR-5744] Refactoring action to convert if-let to guard-let and vice versa#24566
nkcsgexi merged 2 commits into
swiftlang:masterfrom
Regno:feature/vlasov/SR-5744

Conversation

@Regno
Copy link
Copy Markdown
Contributor

@Regno Regno commented May 7, 2019

Implement action to convert from:

func foo(idxOpt: Int?) {
  if let idx = idxOpt {
    print(idx)
  }
}

to

func foo(idxOpt: Int?) {
  guard let idx = idxOpt else {
    return
  }
  print(idx)
}

and vice-versa

Resolves new feature request SR-5744.
Refactoring action to convert if-let to guard-let and vice versa

Feedback is very much appreciated!

@Regno
Copy link
Copy Markdown
Contributor Author

Regno commented May 7, 2019

@swift-ci test

@theblixguy
Copy link
Copy Markdown
Member

You can't invoke CI yourself, you need commit access for it.

@theblixguy
Copy link
Copy Markdown
Member

cc @akyrtzi

@akyrtzi akyrtzi requested a review from nkcsgexi May 14, 2019 00:37
Copy link
Copy Markdown
Contributor

@nkcsgexi nkcsgexi left a comment

Choose a reason for hiding this comment

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

The implementation looks great overall👍! I have only several nitpicks inline.

Comment thread lib/IDE/Refactoring.cpp Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We should always use 2-spaces indentation. Could you fix other indentation issues as well in this change?

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.

Fixed.

Comment thread lib/IDE/Refactoring.cpp Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We can assume performChange() is called only when isApplicable returns true. So it seems the above logics are unnecessary.

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.

Removed.

Comment thread lib/IDE/Refactoring.cpp Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could you move the ThenStmt check to the isApplicable function too? so we don't need to check it here and surprising the users by aborting.

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.

Moved to the isApplicable.

Comment thread lib/IDE/Refactoring.cpp Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

These two statements can be merged as if (auto *ElseBody = dyn_cast_or_null<BraceStmt>(If->getElseStmt())) {}

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.

Fixed.

Comment thread lib/IDE/Refactoring.cpp Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Similarly, can we move these condition checkings into isApplicable function and only check it there?

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.

Moved.

Comment thread lib/IDE/Refactoring.cpp Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Indentation issue here and all other places.

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.

Fixed.

@nkcsgexi
Copy link
Copy Markdown
Contributor

@swift-ci please smoke test

@nkcsgexi
Copy link
Copy Markdown
Contributor

@swift-ci please stress test

@Regno
Copy link
Copy Markdown
Contributor Author

Regno commented Jul 11, 2019

Also I fixed similar issues at this PR: Refactoring action to convert to computed property.

@nkcsgexi
Copy link
Copy Markdown
Contributor

All tests passed. Congratulations for your first Swift contribution!🚢

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants