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

Account for super keyword in SpacingAroundParensRule #373

Merged
merged 3 commits into from
Apr 10, 2019

Conversation

shashachu
Copy link
Contributor

  • When checking for unexpected spaces preceding a (, the rule was only checking elementType IDENTIFIER, so added a check for SUPER_KEYWORD
  • Updated lint and formatting tests

* When checking for unexpected spaces preceding a `(`, the rule was only checking elementType `IDENTIFIER`, so added a check for `SUPER_KEYWORD`
* Updated lint and formatting tests
@shashachu shashachu changed the title Account for super keyword in SpacingAroundParensRule - Fixes #369 Account for super keyword in SpacingAroundParensRule Apr 9, 2019
@shashachu
Copy link
Contributor Author

Fixes #369

@@ -23,7 +29,11 @@ class SpacingAroundParensRule : Rule("paren-spacing") {
val nextLeaf = node.nextLeaf()
val spacingBefore = if (node.elementType == LPAR) {
prevLeaf is PsiWhiteSpace && !prevLeaf.textContains('\n') &&
prevLeaf.prevLeaf()?.elementType == IDENTIFIER && (
(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This formatting looks pretty wacky to me, but AS seems to agree it's correct.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's weird the this open paren is on a new line but the open paren on line 36 is not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I agree. There's definitely bugs with the indent formatting rule but I can't always figure them out.

Copy link

@mbeattie4 mbeattie4 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@NightlyNexus NightlyNexus left a comment

Choose a reason for hiding this comment

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

Test case?

/**
* Ensures there are no extra spaces around parentheses.
*
* See https://kotlinlang.org/docs/reference/coding-conventions.html, "Horizontal Whitespace"
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah yes I was looking for that and just looked through it. will update.

@@ -23,7 +29,11 @@ class SpacingAroundParensRule : Rule("paren-spacing") {
val nextLeaf = node.nextLeaf()
val spacingBefore = if (node.elementType == LPAR) {
prevLeaf is PsiWhiteSpace && !prevLeaf.textContains('\n') &&
prevLeaf.prevLeaf()?.elementType == IDENTIFIER && (
(
Copy link
Contributor

Choose a reason for hiding this comment

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

It's weird the this open paren is on a new line but the open paren on line 36 is not.

@shashachu
Copy link
Contributor Author

Test case?

Tests were added to the lint and format spec files.

Copy link
Contributor

@NightlyNexus NightlyNexus left a comment

Choose a reason for hiding this comment

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

nice.

@shashachu shashachu merged commit 9111baf into pinterest:master Apr 10, 2019
@shashachu shashachu deleted the super-spacing branch April 10, 2019 00:12
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.

5 participants