You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@kyuubi.apache.org by GitBox <gi...@apache.org> on 2022/12/02 14:37:16 UTC

[GitHub] [incubator-kyuubi] bowenliang123 opened a new pull request, #3891: [Improvement] Prefer `binPack` style for imports selector in scalafmt styling

bowenliang123 opened a new pull request, #3891:
URL: https://github.com/apache/incubator-kyuubi/pull/3891

   <!--
   Thanks for sending a pull request!
   
   Here are some tips for you:
     1. If this is your first time, please read our contributor guidelines: https://kyuubi.readthedocs.io/en/latest/community/CONTRIBUTING.html
     2. If the PR is related to an issue in https://github.com/apache/incubator-kyuubi/issues, add '[KYUUBI #XXXX]' in your PR title, e.g., '[KYUUBI #XXXX] Your PR title ...'.
     3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][KYUUBI #XXXX] Your PR title ...'.
   -->
   
   ### _Why are the changes needed?_
   <!--
   Please clarify why the changes are needed. For instance,
     1. If you add a feature, you can talk about the use case of it.
     2. If you fix a bug, you can clarify why it is a bug.
   -->
   to close #3890 .
   
   Change `importSelectors = singleLine` to `importSelectors = binPack` in `.scalafmt.conf`.
   
   
   
   
   ### _How was this patch tested?_
   - [ ] Add some test cases that check the changes thoroughly including negative and positive cases if possible
   
   - [ ] Add screenshots for manual tests if appropriate
   
   - [ x ] [Run test](https://kyuubi.apache.org/docs/latest/develop_tools/testing.html#running-tests) locally before make a pull request
   


-- 
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@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org

[GitHub] [incubator-kyuubi] turboFei commented on pull request #3891: [KYUUBI #3890] [Improvement] Prefer `binPack` style for importSelectors in scalafmt styling

Posted by GitBox <gi...@apache.org>.
turboFei commented on PR #3891:
URL: https://github.com/apache/incubator-kyuubi/pull/3891#issuecomment-1336114313

   > 1. `singleLine` style for imports does force the line of long imports to break `maxColumn` rule, which is not self-explicable and brings worse readability in practical use
   
   1. For the scala code style setting, it allows to ignore the import part.
   ```
     <check level="error" class="org.scalastyle.file.FileLineLengthChecker" enabled="true">
       <parameters>
         <parameter name="maxLineLength"><![CDATA[100]]></parameter>
         <parameter name="tabSize"><![CDATA[2]]></parameter>
         <parameter name="ignoreImports">true</parameter>
       </parameters>
     </check>
   ```
   2. `import` is not code, so it is reasonable to limit the import length.
   3. for a corner case, if my code package depth is big, how do you prevent it to exceed the maxLineLength?


-- 
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@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org

[GitHub] [incubator-kyuubi] bowenliang123 commented on pull request #3891: [KYUUBI #3890] [Improvement] Prefer `binPack` style for importSelectors in scalafmt styling

Posted by GitBox <gi...@apache.org>.
bowenliang123 commented on PR #3891:
URL: https://github.com/apache/incubator-kyuubi/pull/3891#issuecomment-1336112989

   What do you think. @pan3793 @yaooqinn 
   Which is more important self-explicable rules and readability in single screen VS single line for minimum line breaks?


-- 
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@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org

[GitHub] [incubator-kyuubi] bowenliang123 closed pull request #3891: [KYUUBI #3890] [Improvement] Prefer `binPack` style for importSelectors in scalafmt styling

Posted by GitBox <gi...@apache.org>.
bowenliang123 closed pull request #3891: [KYUUBI #3890] [Improvement] Prefer `binPack` style for importSelectors in scalafmt styling
URL: https://github.com/apache/incubator-kyuubi/pull/3891


-- 
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@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org

[GitHub] [incubator-kyuubi] codecov-commenter commented on pull request #3891: [KYUUBI #3890] [Improvement] Prefer `binPack` style for importSelectors in scalafmt styling

Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #3891:
URL: https://github.com/apache/incubator-kyuubi/pull/3891#issuecomment-1335451667

   # [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/3891?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#3891](https://codecov.io/gh/apache/incubator-kyuubi/pull/3891?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d75a075) into [master](https://codecov.io/gh/apache/incubator-kyuubi/commit/00c27a42e3d29d6ce555fd71757e2ae1b843cd9a?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (00c27a4) will **decrease** coverage by `0.00%`.
   > The diff coverage is `n/a`.
   
   ```diff
   @@             Coverage Diff              @@
   ##             master    #3891      +/-   ##
   ============================================
   - Coverage     51.79%   51.78%   -0.01%     
     Complexity       13       13              
   ============================================
     Files           506      506              
     Lines         28955    28955              
     Branches       3987     3987              
   ============================================
   - Hits          14996    14995       -1     
   - Misses        12538    12539       +1     
     Partials       1421     1421              
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-kyuubi/pull/3891?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [...g/apache/kyuubi/sql/KyuubiEnsureRequirements.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3891/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXh0ZW5zaW9ucy9zcGFyay9reXV1YmktZXh0ZW5zaW9uLXNwYXJrLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zcWwvS3l1dWJpRW5zdXJlUmVxdWlyZW1lbnRzLnNjYWxh) | `0.00% <ø> (ø)` | |
   | [...pache/kyuubi/sql/KyuubiQueryStagePreparation.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3891/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXh0ZW5zaW9ucy9zcGFyay9reXV1YmktZXh0ZW5zaW9uLXNwYXJrLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zcWwvS3l1dWJpUXVlcnlTdGFnZVByZXBhcmF0aW9uLnNjYWxh) | `0.00% <ø> (ø)` | |
   | [...g/apache/kyuubi/sql/KyuubiSparkSQLAstBuilder.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3891/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXh0ZW5zaW9ucy9zcGFyay9reXV1YmktZXh0ZW5zaW9uLXNwYXJrLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zcWwvS3l1dWJpU3BhcmtTUUxBc3RCdWlsZGVyLnNjYWxh) | `0.00% <ø> (ø)` | |
   | [...ache/kyuubi/sql/RepartitionBeforeWritingBase.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3891/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXh0ZW5zaW9ucy9zcGFyay9reXV1YmktZXh0ZW5zaW9uLXNwYXJrLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zcWwvUmVwYXJ0aXRpb25CZWZvcmVXcml0aW5nQmFzZS5zY2FsYQ==) | `0.00% <ø> (ø)` | |
   | [...che/kyuubi/sql/watchdog/MaxPartitionStrategy.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3891/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXh0ZW5zaW9ucy9zcGFyay9reXV1YmktZXh0ZW5zaW9uLXNwYXJrLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zcWwvd2F0Y2hkb2cvTWF4UGFydGl0aW9uU3RyYXRlZ3kuc2NhbGE=) | `0.00% <ø> (ø)` | |
   | [...ubi/sql/zorder/InsertZorderBeforeWritingBase.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3891/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXh0ZW5zaW9ucy9zcGFyay9reXV1YmktZXh0ZW5zaW9uLXNwYXJrLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zcWwvem9yZGVyL0luc2VydFpvcmRlckJlZm9yZVdyaXRpbmdCYXNlLnNjYWxh) | `0.00% <ø> (ø)` | |
   | [...rg/apache/kyuubi/sql/zorder/ZorderBytesUtils.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3891/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXh0ZW5zaW9ucy9zcGFyay9reXV1YmktZXh0ZW5zaW9uLXNwYXJrLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zcWwvem9yZGVyL1pvcmRlckJ5dGVzVXRpbHMuc2NhbGE=) | `0.00% <ø> (ø)` | |
   | [...che/spark/sql/PruneFileSourcePartitionHelper.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3891/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXh0ZW5zaW9ucy9zcGFyay9reXV1YmktZXh0ZW5zaW9uLXNwYXJrLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL3NwYXJrL3NxbC9QcnVuZUZpbGVTb3VyY2VQYXJ0aXRpb25IZWxwZXIuc2NhbGE=) | `0.00% <ø> (ø)` | |
   | [...uthz/ranger/RuleApplyRowFilterAndDataMasking.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3891/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXh0ZW5zaW9ucy9zcGFyay9reXV1Ymktc3BhcmstYXV0aHovc3JjL21haW4vc2NhbGEvb3JnL2FwYWNoZS9reXV1YmkvcGx1Z2luL3NwYXJrL2F1dGh6L3Jhbmdlci9SdWxlQXBwbHlSb3dGaWx0ZXJBbmREYXRhTWFza2luZy5zY2FsYQ==) | `91.66% <ø> (ø)` | |
   | [...k/authz/ranger/RuleReplaceShowObjectCommands.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3891/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXh0ZW5zaW9ucy9zcGFyay9reXV1Ymktc3BhcmstYXV0aHovc3JjL21haW4vc2NhbGEvb3JnL2FwYWNoZS9reXV1YmkvcGx1Z2luL3NwYXJrL2F1dGh6L3Jhbmdlci9SdWxlUmVwbGFjZVNob3dPYmplY3RDb21tYW5kcy5zY2FsYQ==) | `62.90% <ø> (ø)` | |
   | ... and [43 more](https://codecov.io/gh/apache/incubator-kyuubi/pull/3891/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   :mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   


-- 
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@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org

[GitHub] [incubator-kyuubi] turboFei commented on pull request #3891: [KYUUBI #3890] [Improvement] Prefer `binPack` style for importSelectors in scalafmt styling

Posted by GitBox <gi...@apache.org>.
turboFei commented on PR #3891:
URL: https://github.com/apache/incubator-kyuubi/pull/3891#issuecomment-1336097083

   emmm, but it looks ugly for scala code. sorry for the -1


-- 
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@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org

[GitHub] [incubator-kyuubi] bowenliang123 commented on pull request #3891: [KYUUBI #3890] [Improvement] Prefer `binPack` style for importSelectors in scalafmt styling

Posted by GitBox <gi...@apache.org>.
bowenliang123 commented on PR #3891:
URL: https://github.com/apache/incubator-kyuubi/pull/3891#issuecomment-1336112706

   Let me try to explain this PR in some more reasons,
   1. `singleLine` style for imports does force the line of long imports to break `maxColumn` rule, which is not self-explicable and brings worse readability in practical use
   2. long imports are unavoidable in some cases, or using `import any._` with wildcard will cause cyclic dependencies (e.g. v2commands.scala as the example the issue mentioned)
   3. `binPack` style won't impact the multi-imports which is not over-sized
   4. `binPack` sytle is the original capability from `scalafmt`, and wont introduced extra mannual workload for auto linting and reformat using IDE or tools.


-- 
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@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org

[GitHub] [incubator-kyuubi] pan3793 commented on pull request #3891: [KYUUBI #3890] [Improvement] Prefer `binPack` style for importSelectors in scalafmt styling

Posted by GitBox <gi...@apache.org>.
pan3793 commented on PR #3891:
URL: https://github.com/apache/incubator-kyuubi/pull/3891#issuecomment-1336125016

   I prefer to keep it as-is.


-- 
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@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org

[GitHub] [incubator-kyuubi] turboFei commented on pull request #3891: [KYUUBI #3890] [Improvement] Prefer `binPack` style for importSelectors in scalafmt styling

Posted by GitBox <gi...@apache.org>.
turboFei commented on PR #3891:
URL: https://github.com/apache/incubator-kyuubi/pull/3891#issuecomment-1336114879

   > 2.long imports are unavoidable in some cases, or using import any._ with wildcard will cause cyclic dependencies (e.g. v2commands.scala as the example the issue mentioned)
   
   I wonder that you can split the multiple imports into two lines manually?
   
   FYI:
   
   Before:
   ```
   import org.apache.kyuubi.plugin.spark.authz.v2Commands.CommandType.{CommandType, HasChildAsIdentifier, HasQueryAsLogicalPlan, HasTableAsIdentifier, HasTableAsIdentifierOption, HasTableNameAsIdentifier}
   ```
   After:
   ```
   import org.apache.kyuubi.plugin.spark.authz.v2Commands.CommandType.{CommandType, HasChildAsIdentifier, HasQueryAsLogicalPlan}
   import org.apache.kyuubi.plugin.spark.authz.v2Commands.CommandType.{HasTableAsIdentifier, HasTableAsIdentifierOption, HasTableNameAsIdentifier}
   ```
   
   
   <img width="1196" alt="image" src="https://user-images.githubusercontent.com/6757692/205432273-dab01111-6d46-4127-b760-a8433adce517.png">
   


-- 
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@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org

[GitHub] [incubator-kyuubi] turboFei commented on pull request #3891: [KYUUBI #3890] [Improvement] Prefer `binPack` style for importSelectors in scalafmt styling

Posted by GitBox <gi...@apache.org>.
turboFei commented on PR #3891:
URL: https://github.com/apache/incubator-kyuubi/pull/3891#issuecomment-1336115146

   > 3. binPack style won't impact the multi-imports which is not over-sized
   It looks not good for this case, at least, I think we should not limit the import length with the same size of code.
   <img width="1076" alt="image" src="https://user-images.githubusercontent.com/6757692/205432332-0e9ceda8-f3b1-4a55-857f-787d93c3a8d3.png">
   


-- 
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@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org

[GitHub] [incubator-kyuubi] bowenliang123 commented on pull request #3891: [KYUUBI #3890] [Improvement] Prefer `binPack` style for importSelectors in scalafmt styling

Posted by GitBox <gi...@apache.org>.
bowenliang123 commented on PR #3891:
URL: https://github.com/apache/incubator-kyuubi/pull/3891#issuecomment-1336128457

   OK, closing this PR. Based on the discussion, the advantage achieved by binpack style for important is not significant and not part of consensu.
   Thanks for the reviews.


-- 
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@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org