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

Fixes/2017/ignore serial version UI d #2045

Merged

Conversation

atulgpt
Copy link
Contributor

@atulgpt atulgpt commented May 19, 2023

Description

Filter the serialversionUID from the analysis
Fixes #2017

Checklist

  • PR description added
  • tests are added
  • KtLint has been applied on source code itself and violations are fixed
  • documentation is updated
  • CHANGELOG.md is updated

Copy link
Collaborator

@paul-dingemans paul-dingemans left a comment

Choose a reason for hiding this comment

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

This PR contains commits abfbbfa and e6ca5a7 which do not belong to this PR. Please ensure that a PR only contains changes relevant to that PR.

@paul-dingemans paul-dingemans added this to the 1.0 (Yeah!) milestone May 20, 2023
@paul-dingemans paul-dingemans marked this pull request as draft May 20, 2023 10:12
@atulgpt atulgpt force-pushed the fixes/2017/ignore-serialVersionUID branch from 45c1ab9 to 2d51a69 Compare May 20, 2023 10:17
@atulgpt atulgpt marked this pull request as ready for review May 20, 2023 10:19
@atulgpt atulgpt force-pushed the fixes/2017/ignore-serialVersionUID branch from 4902325 to bf9eb22 Compare May 20, 2023 12:09
@paul-dingemans
Copy link
Collaborator

I have update your PR.

Next time, please update changelog.

Also, in chained methods please write:

identifier
    .foo {
       
    }?.bar {

   }

instead of

identifier
    .foo {
       
    } // ==> should be merged with next line         
    ?.bar {

   }

@paul-dingemans
Copy link
Collaborator

I can (will) not merge this PR. It still contains changes of another PR which results in merge conflicts above. Please start a new PR and only commit the relevant changes to that PR. Please whenever you start a new PR, please be sure to create a new branch from latest remote master.

@atulgpt
Copy link
Contributor Author

atulgpt commented May 21, 2023

I can (will) not merge this PR. It still contains changes of another PR which results in merge conflicts above. Please start a new PR and only commit the relevant changes to that PR. Please whenever you start a new PR, please be sure to create a new branch from the latest remote master.

Hi, @paul-dingemans sorry for the inconvenience. let me look into it. If you see the file changes you will see that there is no changes from the previous PR. And if I remember correctly I dropped those commits hard pushed yesterday. But anyway let me look into it again

@atulgpt atulgpt force-pushed the fixes/2017/ignore-serialVersionUID branch from a0f9c69 to d86bbec Compare May 21, 2023 12:45
@atulgpt
Copy link
Contributor Author

atulgpt commented May 21, 2023

identifier
.foo {

} // ==> should be merged with next line         
?.bar {

}

Hi @paul-dingemans just to be clear you are suggesting to use below style?

identifier
    .foo {
       
    }?.bar {

   }

I am asking as the existing code in PropertyNamingRule uses below style

node
    .takeIf { node.elementType == PROPERTY }
    ?.let { property -> visitProperty(property, emit) }

@paul-dingemans
Copy link
Collaborator

No, I meant that

}
?.

Should be on same line, like

}?.

@paul-dingemans paul-dingemans merged commit c77574e into pinterest:master May 21, 2023
8 checks passed
@atulgpt atulgpt deleted the fixes/2017/ignore-serialVersionUID branch May 21, 2023 20:08
@atulgpt atulgpt mentioned this pull request Jun 16, 2023
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.

property-naming should exclude serialVersionUID
2 participants