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

build: also aggregate httpScalafix, so headerCheck will also work on those files #245

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

jrudolph
Copy link
Contributor

Should fail until #243 is merged

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

@jrudolph
Copy link
Contributor Author

Also discovered a bug in the test itself and also a bug in sbt-scalafix...

scalacenter/scalafix#1828

Copy link
Member

@He-Pin He-Pin left a comment

Choose a reason for hiding this comment

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

lgtm

@@ -1,3 +1,7 @@
/*
rule = MigrateToServerBuilder # This has to be at the top
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue with running the scalafix test was that the license header must not be above the rule header.

@pjfanning pjfanning added the licensing licensing issues label Jul 19, 2023
@jrudolph jrudolph marked this pull request as draft July 24, 2023 11:07
@jrudolph
Copy link
Contributor Author

fyi. this is still failing because the validatePullRequest task has problems aggregating correctly over modules that have no Scala 3 support and thus fails when encountering the scalafix modules.

@mdedetrich
Copy link
Contributor

This is something that's been on the top of my head and probably should be discussed in general, but I honestly think that validatePullRequest should be removed because its overly complex and most of the problems that it catches are already solved by other mechanisms (i.e. strict github checks) which likely didn't even exist when it was created.

@jrudolph
Copy link
Contributor Author

It's not about the problems it solves but about how much time is saved by preventing to execute extra tests.

But I agree about the complexity, so if we can simplify without losing the ability to run a reduced set of tests, than that would be nice. Might be that most complexity is for the extra reporting which is basically related to reporting a good summary on Github while we were still running on Jenkins.

@pjfanning
Copy link
Contributor

@jrudolph @mdedetrich do we think this is needed for 1.0.0-RC2?

Other than upgrading to Pekko (Core) 1.0.1 when it is released, I don't think we need any more changes for Pekko HTTP 1.0.0-RC2.

@jrudolph
Copy link
Contributor Author

@jrudolph @mdedetrich do we think this is needed for 1.0.0-RC2?

No, this is just an improvement to avoid overlooking issues in those modules in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
licensing licensing issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants