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/11/14 09:48:55 UTC
[GitHub] [incubator-pekko-http] mdedetrich opened a new pull request, #8: Update and apply scalafmt
mdedetrich opened a new pull request, #8:
URL: https://github.com/apache/incubator-pekko-http/pull/8
Update same scalafmt as in pekko core. @jrudolph If you want to have a look. Note that both the scalafmt configuration and the PR in general is the exact same as has been done in other repos, this is being done for the sake of consistency
--
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 merged pull request #8: Update and apply scalafmt
Posted by GitBox <gi...@apache.org>.
mdedetrich merged PR #8:
URL: https://github.com/apache/incubator-pekko-http/pull/8
--
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 #8: Update and apply scalafmt
Posted by GitBox <gi...@apache.org>.
mdedetrich commented on PR #8:
URL: https://github.com/apache/incubator-pekko-http/pull/8#issuecomment-1313486134
Okay I fixed the paradox problem, PR is good to go.
--
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 #8: Update and apply scalafmt
Posted by GitBox <gi...@apache.org>.
jrudolph commented on PR #8:
URL: https://github.com/apache/incubator-pekko-http/pull/8#issuecomment-1313446671
> I am not too familiar with the goal of this task is but is it something that is safe to ignore?
No, it cannot be ignored, it's the main validation task. It failed while running `docs/paradox`. It seems the formatting changed the directive signatures enough that they are not picked up any more by the documentation.
https://github.com/apache/incubator-pekko-http/actions/runs/3460492533/jobs/5777086414#step:7:4597
--
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 #8: Update and apply scalafmt
Posted by GitBox <gi...@apache.org>.
mdedetrich commented on code in PR #8:
URL: https://github.com/apache/incubator-pekko-http/pull/8#discussion_r1021304661
##########
.github/workflows/validate-and-test.yml:
##########
@@ -33,8 +33,8 @@ jobs:
path: project/**/target
key: build-target-${{ hashFiles('**/*.sbt', 'project/build.properties', 'project/**/*.scala') }}
- - name: Autoformat
- run: sbt +headerCreateAll +scalariformFormat +test:scalariformFormat
+ - name: Check code is formatted
+ run: sbt scalafmtCheckAll scalafmtSbtCheck headerCheckAll
Review Comment:
So this is just temporary, i.e. rather than doing an auto format we just check that the code is formatted.
This will be replaced in the future with the scalafmt-native github action
--
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 #8: Update and apply scalafmt
Posted by GitBox <gi...@apache.org>.
mdedetrich commented on PR #8:
URL: https://github.com/apache/incubator-pekko-http/pull/8#issuecomment-1316549795
I will merge this PR by end of day if there are no pressing comments incase there are going to be future contributions (and to then avoid annoying merge conflicts).
I am quite sure the current implementation works without issues, the only case where it won't work is if you use default parameters in the function definition signatures but tbh to do a proper solution it would probably best to use scalameta to parse the source code and a pretty printer to render the AST for the documentation (and in such a case it should be contributed to sbt-paradox).
--
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 #8: Update and apply scalafmt
Posted by GitBox <gi...@apache.org>.
jrudolph commented on PR #8:
URL: https://github.com/apache/incubator-pekko-http/pull/8#issuecomment-1313557016
> Okay I fixed the paradox problem, PR is good to go.
Isn't that just breaking the documentation? Is the plan to fix it 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 #8: Update and apply scalafmt
Posted by GitBox <gi...@apache.org>.
mdedetrich commented on PR #8:
URL: https://github.com/apache/incubator-pekko-http/pull/8#issuecomment-1313441075
@pjfanning @jrudolph So it turns out the `validatePullRequest` has failed, I am not too familiar with the goal of this task is but is it something that is safe to ignore?
--
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 #8: Update and apply scalafmt
Posted by GitBox <gi...@apache.org>.
mdedetrich commented on PR #8:
URL: https://github.com/apache/incubator-pekko-http/pull/8#issuecomment-1313682592
Thanks for the explanation, will put in a fix tonight.
--
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 #8: Update and apply scalafmt
Posted by GitBox <gi...@apache.org>.
mdedetrich commented on PR #8:
URL: https://github.com/apache/incubator-pekko-http/pull/8#issuecomment-1314267519
@jrudolph I just rebased the PR with a solution, see the `Fix paradox issues caused by scalafmt` commit.
--
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 #8: Update and apply scalafmt
Posted by GitBox <gi...@apache.org>.
mdedetrich commented on PR #8:
URL: https://github.com/apache/incubator-pekko-http/pull/8#issuecomment-1316786937
I just realized that due to whats happening both with https://github.com/apache/incubator-pekko/pull/50 and https://github.com/apache/incubator-pekko/issues/38#issuecomment-1311468140 I might have to restructure the PR. Specifically the change to `project/ParadoxSupport.scala` in the `Fix paradox issues caused by scalafmt` commit requires a update to the header and this is not possible to do right now because we have a header check that will break if there isn't a standard lightbend header.
For this reason I will do the equivalent of https://github.com/apache/incubator-pekko/pull/50 in pekko-http before merging this PR and I will then update the `Fix paradox issues caused by scalafmt` to work with the new combined header.
--
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 #8: Update and apply scalafmt
Posted by GitBox <gi...@apache.org>.
mdedetrich commented on PR #8:
URL: https://github.com/apache/incubator-pekko-http/pull/8#issuecomment-1313456787
Ah yes I see it now, looking into 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] mdedetrich commented on a diff in pull request #8: Update and apply scalafmt
Posted by GitBox <gi...@apache.org>.
mdedetrich commented on code in PR #8:
URL: https://github.com/apache/incubator-pekko-http/pull/8#discussion_r1021304661
##########
.github/workflows/validate-and-test.yml:
##########
@@ -33,8 +33,8 @@ jobs:
path: project/**/target
key: build-target-${{ hashFiles('**/*.sbt', 'project/build.properties', 'project/**/*.scala') }}
- - name: Autoformat
- run: sbt +headerCreateAll +scalariformFormat +test:scalariformFormat
+ - name: Check code is formatted
+ run: sbt scalafmtCheckAll scalafmtSbtCheck headerCheckAll
Review Comment:
So this is just temporary, i.e. rather than doing an auto format we just check that the code is formatted.
This will be replaced in the future with the scalafmt-native github action (`headerCheckAll` will still remain).
--
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 #8: Update and apply scalafmt
Posted by GitBox <gi...@apache.org>.
jrudolph commented on PR #8:
URL: https://github.com/apache/incubator-pekko-http/pull/8#issuecomment-1313679993
> If there is some problem paradox should fail to compile (which was the original problem) and the issue that scalafmt put forward is it puts a ` ` (space) after comments, specifically `#` which is what the `Fix paradox issues caused by scalafmt whitespace` is meant to solve.
In this case, `#xyz` in markdown is a reference to a method of the source file. However, the code that extracts signatures from source files does currently not have support for multi-line signatures, this is why the check failed before.
Now the whole markdown directive is not parsed anymore and is inserted verbatim into the generated document (markdown is hard to parse so that's one of the usual drawbacks...):
The fix would be either to revert and fixate the signatures in question to keep using the single-line format (using no format or special rules for the source files in question). Or, in `project/ParadoxSupport` the custom `@@signature` directive implementation could be improved to support multi-line signatures (probably the correct fix).
--
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 #8: Update and apply scalafmt
Posted by GitBox <gi...@apache.org>.
mdedetrich commented on PR #8:
URL: https://github.com/apache/incubator-pekko-http/pull/8#issuecomment-1313642778
> Isn't that just breaking the documentation? Is the plan to fix it later?
So as far as my understanding of paradox is this should have fixed it. If there is some problem paradox should fail to compile (which was the original problem) and the issue that scalafmt put forward is it puts a ` ` (space) after comments, specifically `#` which is what the `Fix paradox issues caused by scalafmt whitespace` is meant to solve.
I will double check locally that the documentation is equivalent
--
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 #8: Update and apply scalafmt
Posted by GitBox <gi...@apache.org>.
mdedetrich commented on PR #8:
URL: https://github.com/apache/incubator-pekko-http/pull/8#issuecomment-1319643557
Considering the discussion on the mailing list, I am going to merge this PR. If header changes are required because its considered a major change this can be updated 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