-
Notifications
You must be signed in to change notification settings - Fork 39
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Also discovered a bug in the test itself and also a bug in sbt-scalafix... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
6da7879
to
385220d
Compare
@@ -1,3 +1,7 @@ | |||
/* | |||
rule = MigrateToServerBuilder # This has to be at the top |
There was a problem hiding this comment.
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.
fyi. this is still failing because the |
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. |
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. |
@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. |
No, this is just an improvement to avoid overlooking issues in those modules in the future. |
Should fail until #243 is merged