You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@pekko.apache.org by GitBox <gi...@apache.org> on 2022/12/10 13:02:02 UTC
[GitHub] [incubator-pekko-http] luksow opened a new pull request, #14: moving to upstream parboiled2 dependency
luksow opened a new pull request, #14:
URL: https://github.com/apache/incubator-pekko-http/pull/14
Hi,
since https://github.com/apache/incubator-pekko-http/issues/1 was rather easy, I took stab at it. The parboiled2 interface changed a bit over the time, so more changes were needed than just the imports.
Thing to consider is to remove akka-parsing completely but I think it was partially sorted out in https://github.com/apache/incubator-pekko-http/tree/scala-3 branch
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] jrudolph commented on a diff in pull request #14: moving to upstream parboiled2 dependency
Posted by "jrudolph (via GitHub)" <gi...@apache.org>.
jrudolph commented on code in PR #14:
URL: https://github.com/apache/incubator-pekko-http/pull/14#discussion_r1105714362
##########
http-core/src/main/scala/org/apache/pekko/http/impl/model/parser/HeaderParser.scala:
##########
@@ -110,11 +110,11 @@ private[http] object HeaderParser {
object EmptyCookieException extends SingletonException("Cookie header contained no parsable cookie values.")
def lookupParser(headerName: String, settings: Settings = DefaultSettings): Option[String => HeaderParser#Result] =
- dispatch.lookup(headerName).map { runner => (value: String) =>
- import pekko.parboiled2.EOI
+ Some { (value: String) =>
Review Comment:
I changed that to keep the old signatures even if the new one might "lie" because it may still fail later with a `RuleNotFound` exception (only if we are adding ModeledHeaders without accompanying it with the correctly named parser).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] jrudolph commented on pull request #14: moving to upstream parboiled2 dependency
Posted by "jrudolph (via GitHub)" <gi...@apache.org>.
jrudolph commented on PR #14:
URL: https://github.com/apache/incubator-pekko-http/pull/14#issuecomment-1439644498
Cleaned up the commits into a single one based on top of current master.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] jrudolph commented on a diff in pull request #14: moving to upstream parboiled2 dependency
Posted by "jrudolph (via GitHub)" <gi...@apache.org>.
jrudolph commented on code in PR #14:
URL: https://github.com/apache/incubator-pekko-http/pull/14#discussion_r1105718777
##########
pekko-http-core/src/main/scala/akka/http/impl/model/parser/HeaderParser.scala:
##########
@@ -108,25 +108,25 @@ private[http] object HeaderParser {
object EmptyCookieException extends SingletonException("Cookie header contained no parsable cookie values.")
- def lookupParser(headerName: String, settings: Settings = DefaultSettings): Option[String => HeaderParser#Result] =
- dispatch.lookup(headerName).map { runner => (value: String) =>
Review Comment:
Created https://github.com/apache/incubator-pekko-http/issues/45
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] mdedetrich commented on pull request #14: moving to upstream parboiled2 dependency
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #14:
URL: https://github.com/apache/incubator-pekko-http/pull/14#issuecomment-1429624428
> I added a merge commit to fix the conflicts.
Thanks, I just quickly checked the PR and I don't see any class files containing Akka being modified so there shouldn't be any major merge conflicts later
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] mdedetrich commented on pull request #14: moving to upstream parboiled2 dependency
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #14:
URL: https://github.com/apache/incubator-pekko-http/pull/14#issuecomment-1430985527
So I noticed that on my phone its fine and on the desktop its still broken, so this is probably the most liberal definition of eventual thatI have seen in a while.
Anyways it just means someone else will have to merge.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] pjfanning commented on pull request #14: moving to upstream parboiled2 dependency
Posted by GitBox <gi...@apache.org>.
pjfanning commented on PR #14:
URL: https://github.com/apache/incubator-pekko-http/pull/14#issuecomment-1345390468
Thanks for the PR. We still need to decide if we want to make this change yet. For v1.0.0, we generally want minimal changes.
The scala-3 branch is shaping up to be the basis of the v1.1.0 release and we may end targeting this work to that release and possibly that branch.
The scalfmt checks are failing and it would be a good idea to change the order of the imports so that the org.parboiled imports are not with the Akka imports.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] jrudolph commented on pull request #14: moving to upstream parboiled2 dependency
Posted by "jrudolph (via GitHub)" <gi...@apache.org>.
jrudolph commented on PR #14:
URL: https://github.com/apache/incubator-pekko-http/pull/14#issuecomment-1441526361
> @luksow this change is large enough that we probably need a CLA. If you don't have one already, could you get an iCLA as documented in [apache.org/licenses/contributor-agreements.html](https://www.apache.org/licenses/contributor-agreements.html) ?
AFAIU, we have to collect CLAs for every contributor. We have the problem here that if we don't get a CLA we will have to resubmit the exact same PR with a new author because it is so trivial that there is no other alternative to these exact changes. After all only 66 lines were updated or added here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] luksow commented on pull request #14: moving to upstream parboiled2 dependency
Posted by "luksow (via GitHub)" <gi...@apache.org>.
luksow commented on PR #14:
URL: https://github.com/apache/incubator-pekko-http/pull/14#issuecomment-1429950527
Wow, you're moving quick! I barely was able to have a look and all work is already done :) Glad to see it moving forward.
LGTM.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] pjfanning commented on pull request #14: moving to upstream parboiled2 dependency
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on PR #14:
URL: https://github.com/apache/incubator-pekko-http/pull/14#issuecomment-1432156599
could we hold off for a couple of days? - on #1, it has been suggested that we run a perf test - may be over cautious but may be no harm if someone has time to run the benchmarks in this project (with and without this change)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] mdedetrich commented on pull request #14: moving to upstream parboiled2 dependency
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #14:
URL: https://github.com/apache/incubator-pekko-http/pull/14#issuecomment-1432158283
Sure
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] luksow commented on pull request #14: moving to upstream parboiled2 dependency
Posted by "luksow (via GitHub)" <gi...@apache.org>.
luksow commented on PR #14:
URL: https://github.com/apache/incubator-pekko-http/pull/14#issuecomment-1429957918
@mdedetrich: here it says it's good to go :thinking:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] jrudolph commented on a diff in pull request #14: moving to upstream parboiled2 dependency
Posted by "jrudolph (via GitHub)" <gi...@apache.org>.
jrudolph commented on code in PR #14:
URL: https://github.com/apache/incubator-pekko-http/pull/14#discussion_r1105712824
##########
http-core/src/main/scala/org/apache/pekko/http/impl/model/parser/CacheControlHeader.scala:
##########
@@ -5,11 +5,10 @@
package org.apache.pekko.http.impl.model.parser
import org.apache.pekko
-import pekko.parboiled2.Parser
import pekko.http.scaladsl.model.headers._
import CacheDirectives._
-private[parser] trait CacheControlHeader { this: Parser with CommonRules with CommonActions with StringBuilding =>
+private[parser] trait CacheControlHeader { this: HeaderParser =>
Review Comment:
Somehow the Scala compiler got confused here about the more restricted self-type, not sure what's the issue, but that works for now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] mdedetrich commented on pull request #14: moving to upstream parboiled2 dependency
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #14:
URL: https://github.com/apache/incubator-pekko-http/pull/14#issuecomment-1431340209
> The reason is probably that you selected "rebase and merge" which isn't possible cleanly due to my merge commit. But we can squash here.
You are right, this is a case of UI ergonomics because the drop down button which lets you change the merge strategy was kind of greyed out (implying it was disabled), probably it being mixed with dark theme did not help.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] jrudolph merged pull request #14: moving to upstream parboiled2 dependency
Posted by "jrudolph (via GitHub)" <gi...@apache.org>.
jrudolph merged PR #14:
URL: https://github.com/apache/incubator-pekko-http/pull/14
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] pjfanning commented on pull request #14: moving to upstream parboiled2 dependency
Posted by GitBox <gi...@apache.org>.
pjfanning commented on PR #14:
URL: https://github.com/apache/incubator-pekko-http/pull/14#issuecomment-1345639940
@mdedetrich, @jrudolph, @nvollmar - I think we should consider treating the scala-3 branch as the v1.1.0 branch. If this PR was retargeted to the scala-3 branch, would any of you support approving the PR?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] pjfanning commented on pull request #14: moving to upstream parboiled2 dependency
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on PR #14:
URL: https://github.com/apache/incubator-pekko-http/pull/14#issuecomment-1441537869
In practice, ASF projects don't collect CLAs from every user who submits a PR. There is no magic criterion for what makes a PR significant enough to require one but the PR changes 70 files, even if the changes are not very large.
Let's see what @luksow says, if he doesn't have time to sort out the CLA, we are probably going to have to look at alternatives.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] mdedetrich commented on pull request #14: moving to upstream parboiled2 dependency
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #14:
URL: https://github.com/apache/incubator-pekko-http/pull/14#issuecomment-1432887850
As per discussion at https://github.com/apache/incubator-pekko-http/issues/1#issuecomment-1432884633 I have applied the `upstream` label (indicating that the PR is waiting for some upstream changes).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] mdedetrich commented on a diff in pull request #14: moving to upstream parboiled2 dependency
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #14:
URL: https://github.com/apache/incubator-pekko-http/pull/14#discussion_r1105709046
##########
pekko-http-core/src/main/scala/akka/http/impl/model/parser/HeaderParser.scala:
##########
@@ -108,25 +108,25 @@ private[http] object HeaderParser {
object EmptyCookieException extends SingletonException("Cookie header contained no parsable cookie values.")
- def lookupParser(headerName: String, settings: Settings = DefaultSettings): Option[String => HeaderParser#Result] =
- dispatch.lookup(headerName).map { runner => (value: String) =>
Review Comment:
Makes sense to me
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] mdedetrich commented on a diff in pull request #14: moving to upstream parboiled2 dependency
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #14:
URL: https://github.com/apache/incubator-pekko-http/pull/14#discussion_r1105711226
##########
pekko-http-core/src/main/scala/akka/http/impl/model/parser/HeaderParser.scala:
##########
@@ -108,25 +108,25 @@ private[http] object HeaderParser {
object EmptyCookieException extends SingletonException("Cookie header contained no parsable cookie values.")
- def lookupParser(headerName: String, settings: Settings = DefaultSettings): Option[String => HeaderParser#Result] =
- dispatch.lookup(headerName).map { runner => (value: String) =>
Review Comment:
> Maybe, let's try to upstream the optimization and go back to the simpler version for now (which adds an extra hashmap lookup per header parsed).
If we do this then lets make an issue on it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] nvollmar commented on pull request #14: moving to upstream parboiled2 dependency
Posted by GitBox <gi...@apache.org>.
nvollmar commented on PR #14:
URL: https://github.com/apache/incubator-pekko-http/pull/14#issuecomment-1345959403
I think that would make sense.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] mdedetrich commented on pull request #14: moving to upstream parboiled2 dependency
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #14:
URL: https://github.com/apache/incubator-pekko-http/pull/14#issuecomment-1441659865
> In practice, ASF projects don't collect CLAs from every user who submits a PR. There is no magic criterion for what makes a PR significant enough to require one but the PR changes 70 files, even if the changes are not very large.
My understanding is also the same, an ICLA is required for significant contributions but the definition of this is entirely subjective.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] pjfanning commented on pull request #14: moving to upstream parboiled2 dependency
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on PR #14:
URL: https://github.com/apache/incubator-pekko-http/pull/14#issuecomment-1440336381
@luksow this change is large enough that we probably need a CLA. If you don't have one already, could you get an iCLA as documented in https://www.apache.org/licenses/contributor-agreements.html ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] jrudolph commented on pull request #14: moving to upstream parboiled2 dependency
Posted by "jrudolph (via GitHub)" <gi...@apache.org>.
jrudolph commented on PR #14:
URL: https://github.com/apache/incubator-pekko-http/pull/14#issuecomment-1439646539
> As per discussion at [#1 (comment)](https://github.com/apache/incubator-pekko-http/issues/1#issuecomment-1432884633) I have applied the `upstream` label (indicating that the PR is waiting for some upstream changes).
I don't think we should hold off for any upstream changes as discussed above and in #1, the performance hit is currently minor and can partly be solved without upstream changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] luksow commented on pull request #14: moving to upstream parboiled2 dependency
Posted by GitBox <gi...@apache.org>.
luksow commented on PR #14:
URL: https://github.com/apache/incubator-pekko-http/pull/14#issuecomment-1345521608
@pjfanning thanks for your comments. I did scalafmt and also reorganized imports so that org.parboiled2 is always after akka's.
After you decide the faith of v1.0.0 - let me know. I'm happy to target v1.1.0 or scala-3 branch instead.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] luksow commented on pull request #14: moving to upstream parboiled2 dependency
Posted by "luksow (via GitHub)" <gi...@apache.org>.
luksow commented on PR #14:
URL: https://github.com/apache/incubator-pekko-http/pull/14#issuecomment-1441571002
@pjfanning @jrudolph I've just signed and sent ICLA.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] jrudolph commented on a diff in pull request #14: moving to upstream parboiled2 dependency
Posted by "jrudolph (via GitHub)" <gi...@apache.org>.
jrudolph commented on code in PR #14:
URL: https://github.com/apache/incubator-pekko-http/pull/14#discussion_r1105708528
##########
pekko-http-core/src/main/scala/akka/http/impl/model/parser/HeaderParser.scala:
##########
@@ -108,25 +108,25 @@ private[http] object HeaderParser {
object EmptyCookieException extends SingletonException("Cookie header contained no parsable cookie values.")
- def lookupParser(headerName: String, settings: Settings = DefaultSettings): Option[String => HeaderParser#Result] =
- dispatch.lookup(headerName).map { runner => (value: String) =>
Review Comment:
Maybe, let's try to upstream the optimization and go back to the simpler version for now (which adds an extra hashmap lookup per header parsed).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] jrudolph commented on a diff in pull request #14: moving to upstream parboiled2 dependency
Posted by "jrudolph (via GitHub)" <gi...@apache.org>.
jrudolph commented on code in PR #14:
URL: https://github.com/apache/incubator-pekko-http/pull/14#discussion_r1105685766
##########
pekko-http-core/src/main/scala/akka/http/impl/model/parser/HeaderParser.scala:
##########
@@ -108,25 +108,25 @@ private[http] object HeaderParser {
object EmptyCookieException extends SingletonException("Cookie header contained no parsable cookie values.")
- def lookupParser(headerName: String, settings: Settings = DefaultSettings): Option[String => HeaderParser#Result] =
- dispatch.lookup(headerName).map { runner => (value: String) =>
Review Comment:
We should keep this optimization, maybe we have to keep at our own version of `DynamicRuleDispatch`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] mdedetrich commented on pull request #14: moving to upstream parboiled2 dependency
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #14:
URL: https://github.com/apache/incubator-pekko-http/pull/14#issuecomment-1429981161
I need to download your version of github :D
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] mdedetrich commented on pull request #14: moving to upstream parboiled2 dependency
Posted by GitBox <gi...@apache.org>.
mdedetrich commented on PR #14:
URL: https://github.com/apache/incubator-pekko-http/pull/14#issuecomment-1346003005
I would also support putting this into 1.1.x, will retarget the PR when the 1.1.x branch is out. @luksow Thanks a lot for the work, much appreciated!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] jrudolph commented on pull request #14: moving to upstream parboiled2 dependency
Posted by "jrudolph (via GitHub)" <gi...@apache.org>.
jrudolph commented on PR #14:
URL: https://github.com/apache/incubator-pekko-http/pull/14#issuecomment-1429622225
I added a merge commit to fix the conflicts.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] mdedetrich commented on pull request #14: moving to upstream parboiled2 dependency
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #14:
URL: https://github.com/apache/incubator-pekko-http/pull/14#issuecomment-1429952665
> Wow, you're moving quick! I barely was able to have a look and all work is already done :) Glad to see it moving forward.
Yeah this is a common problem we have....
There is still one issue though, which is that the PR is unable to be merged because it needs to be rebased due to conflicts (at least thats what github is complaining)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] spangaer commented on pull request #14: moving to upstream parboiled2 dependency
Posted by "spangaer (via GitHub)" <gi...@apache.org>.
spangaer commented on PR #14:
URL: https://github.com/apache/incubator-pekko-http/pull/14#issuecomment-1430927983
Tie breaker
Maybe you should try light mode 😉
Perhaps it's resolved by now, I saw the same yesterday evening, but it's not the first time I notice GitHub being "eventually consistent"
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] jrudolph commented on pull request #14: moving to upstream parboiled2 dependency
Posted by "jrudolph (via GitHub)" <gi...@apache.org>.
jrudolph commented on PR #14:
URL: https://github.com/apache/incubator-pekko-http/pull/14#issuecomment-1431039197
The reason is probably that you selected "rebase and merge" which isn't possible cleanly due to my merge commit. But we can squash here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] mdedetrich commented on pull request #14: moving to upstream parboiled2 dependency
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #14:
URL: https://github.com/apache/incubator-pekko-http/pull/14#issuecomment-1429559125
> @luksow would you consider updating this PR to work with the latest main branch? The `akka` package classes have moved to `org.apache.pekko`. The main branch should be more stable now.
>
> We are actually thinking about merging this into main for the v1.0.0 release.
I was also going to mention this. There are still a couple more big structural changes that need to be done (i.e. configuration change from `akka` to `pekko` as well as renaming types that have the akka in them. I don't think that these changes would cause further merge conflicts for this kind of PR but I am going to focus on this for the week.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] pjfanning commented on pull request #14: moving to upstream parboiled2 dependency
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on PR #14:
URL: https://github.com/apache/incubator-pekko-http/pull/14#issuecomment-1429556362
@luksow would you consider updating this PR to work with the latest main branch? The `akka` package classes have moved to `org.apache.pekko`. The main branch should be more stable now.
We are actually thinking about merging this into main for the v1.0.0 release.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] jrudolph commented on pull request #14: moving to upstream parboiled2 dependency
Posted by "jrudolph (via GitHub)" <gi...@apache.org>.
jrudolph commented on PR #14:
URL: https://github.com/apache/incubator-pekko-http/pull/14#issuecomment-1429658008
(Fixed the test fixes I accidentally reverted from @luksow's state)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org