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

Fix spotlessApply conflict with black #3152

Merged
merged 4 commits into from
Apr 30, 2021

Conversation

ChristopheDuong
Copy link
Contributor

@ChristopheDuong ChristopheDuong commented Apr 30, 2021

What

Trying to make spotlessApply on python files equivalent to black formatting...

How

Spotless for python is adding trailing whitespace in headers which is removed by black producing many diffs if one is run and not the other
(seems airbyte-cdk was checked in with trailing whitespace so I'm removing them)

Recommended reading order

  1. build.gradle
  2. the rest

license.eachLine {
w << lineComment
w.writeLine(it)
}
w.writeLine(endComment)
if (endComment.length() > 0 || !isPython) {
Copy link
Contributor

Choose a reason for hiding this comment

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

for my own undersanding: what is the purpose of these if conditions? might be misunderstanding but it seems like the end state is this will always write for java and not for python, which is the equivalent to what is being set in the start and end comments for either language.

triimTrailingWhitespace() by itself is not enough to get rid of the spaces?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In java, we don't have a second linter/styling tool other than spotless so it doesn't really matter if we write a newline or not for startComment/endComment.

However, in python, we are using black that double-checks and reformats the code.
Thus, writing an extra empty newline (not removed by trimTrailingWhitespace() is actually a big deal and would be reformatted (removed) because in black's specs, there should be exactly one (or two?) empty lines between license header and imports etc

So in python, if you don't use a startComment/endComment syntax, then those lines shouldn't be written at all.
If you do specify some startComment/endComment characters, the lines won't be empty so it should be ok...

Copy link
Contributor

@davinchia davinchia Apr 30, 2021

Choose a reason for hiding this comment

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

Thanks for the explanation!

Makes sense. Can we drop a comment capturing this:

In java, we don't have a second linter/styling tool other than spotless so it doesn't really matter if we write a newline or not for startComment/endComment.

However, in python, we are using black that double-checks and reformats the code.
Thus, writing an extra empty newline (not removed by trimTrailingWhitespace() is actually a big deal and would be reformatted (removed) because in black's specs, there should be exactly one (or two?) empty lines between license header and imports etc

I'm pretty sure I would not remember this after a month heh

Suggestion - maybe simplify the if conditions to just:

if (!isPython) {
            w.writeLine(startComment)
        }
        license.eachLine {
            w << lineComment
            w.writeLine(it)
        }
        if (!isPython) {
            w.writeLine(endComment)
        }

since the python relevant logic is already centralised in createPythonLicenseWith. I can see argue for checking either parameter, so leave this to your judgement.

Copy link
Contributor

@davinchia davinchia left a comment

Choose a reason for hiding this comment

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

tested this locally using ./gradlew :airbyte-integrations:connectors:destination-redshift:build -x test and this seemed to work!

I have one question for my own understanding before approving.

Copy link
Contributor

@davinchia davinchia left a comment

Choose a reason for hiding this comment

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

This is awesome! Thanks Chris!

Copy link
Contributor

@cgardens cgardens left a comment

Choose a reason for hiding this comment

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

Thanks, @ChristopheDuong !!!

@ChristopheDuong ChristopheDuong merged commit 8dff158 into master Apr 30, 2021
@ChristopheDuong ChristopheDuong deleted the chris/fix-spotless-black-diff branch April 30, 2021 19:09
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.

None yet

4 participants