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

Sort stream imports for pekko-stream modules #933

Merged
merged 2 commits into from
Jan 16, 2024

Conversation

He-Pin
Copy link
Member

@He-Pin He-Pin commented Jan 13, 2024

Motivation:
When I do code modification in pekko-stream related module, after a sortImports I have to revert all unrelated changes, which is very annoying.

Result:
Sort the imports in pekko-stream and pekko-stream-tests.

The sort-imports will cause problem when encounter #xxx and does not obey the #scala-fix:off/on too.

"org.reactivestreams."
"io.netty."
"org.scalatest."
"org.slf4j."
"com.typesafe."
Copy link
Member Author

Choose a reason for hiding this comment

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

I updated the conf.

@He-Pin He-Pin requested a review from mdedetrich January 13, 2024 10:10
@mdedetrich
Copy link
Contributor

Are you sorting this using Intellij?

@He-Pin
Copy link
Member Author

He-Pin commented Jan 15, 2024

@mdedetrich I'm using the sbt sortImports

@mdedetrich
Copy link
Contributor

@mdedetrich I'm using the sbt sortImports

Nice work, this is looking good especially considering its not messing with the import org.apache.pekko style and we can add a check to make sure imports are sorted to the CI later. I just added one comment.

@mdedetrich
Copy link
Contributor

Not critical but it also would be good to squash the chore: sort imports for pekko-stream-tests and chore: sort imports for pekko-stream into a single commit.

@laglangyue
Copy link
Contributor

When merging to the main branch, all commits will be converted to a single commit, right?
I often submit multiple times, I just want to make sure if I made a mistake.

Not critical but it also would be good to squash the chore: sort imports for pekko-stream-tests and chore: sort imports for pekko-stream into a single commit.

@laglangyue
Copy link
Contributor

LGTM

@mdedetrich
Copy link
Contributor

mdedetrich commented Jan 15, 2024

When merging to the main branch, all commits will be converted to a single commit, right?
I often submit multiple times, I just want to make sure if I made a mistake.

So this is the whole "rebase and merge" vs "squash and merge" debate. Some people have different preferences, but this PR specifically is an exception because since its adding a format commit we want to add it .git-blame-ignore-revs so it doesn't turn up in git blame however we want to make sure that ONLY the formatting changes are added and not anything else.

So ideally for this PR you would have one commit that does all of the necessary changes i.e. 84fb5f8 and then another commit which applies sortImports and its this commit which gets added to https://github.com/apache/incubator-pekko/blob/main/.git-blame-ignore-revs.

Otherwise we ordinarily don't care too much about rebase + merge versus squash + merge, although if you are the kind of person that adds lots of minor commits then you should use squash + merge.

@He-Pin
Copy link
Member Author

He-Pin commented Jan 15, 2024

@laglangyue I will do a rebase later, it was in this style to make review easier.

@He-Pin
Copy link
Member Author

He-Pin commented Jan 15, 2024

Once this get merged, will follow up a .git-blame-ignore-revs pr.

@He-Pin
Copy link
Member Author

He-Pin commented Jan 15, 2024

@mdedetrich @pjfanning I would like to have this merged first:)

@He-Pin He-Pin force-pushed the sortStreamImports branch from ae53336 to 858b94b Compare January 16, 2024 06:32
@He-Pin He-Pin requested a review from mdedetrich January 16, 2024 06:35
Copy link
Contributor

@mdedetrich mdedetrich left a comment

Choose a reason for hiding this comment

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

lgtm

@He-Pin
Copy link
Member Author

He-Pin commented Jan 16, 2024

Let me merge with with another pr which ignores the commit.

@He-Pin He-Pin merged commit c44c0b7 into apache:main Jan 16, 2024
18 checks passed
@He-Pin He-Pin deleted the sortStreamImports branch January 16, 2024 08:10
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