Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Bump Scalafmt to 3.8.x #6674

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

bowenliang123
Copy link
Contributor

@bowenliang123 bowenliang123 commented Sep 5, 2024

🔍 Description

Issue References 🔗

This pull request fixes #

Describe Your Solution 🔧

Without this PR:

  • use and conform Scalafmt 3.7.x
  • forcing all the imports from the same parent package, which is even violation the max length of the line

With this PR:

Types of changes 🔖

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Test Plan 🧪

Behavior Without This Pull Request ⚰️

Behavior With This Pull Request 🎉

Related Unit Tests


Checklist 📝

Be nice. Be informative.

@codecov-commenter
Copy link

codecov-commenter commented Sep 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 0.00%. Comparing base (dd9cc0e) to head (c302ffc).

Additional details and impacted files
@@          Coverage Diff           @@
##           master   #6674   +/-   ##
======================================
  Coverage    0.00%   0.00%           
======================================
  Files         688     688           
  Lines       42590   42590           
  Branches     5805    5805           
======================================
  Misses      42590   42590           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pan3793
Copy link
Member

pan3793 commented Sep 6, 2024

I'm surprised that scalafmt made such a breaking change. Maybe it's time to discuss Kyuubi 2.0, and we can consider including this change in 2.0 and keeping the existing behavior on 1.x serials.

@bowenliang123
Copy link
Contributor Author

bowenliang123 commented Sep 6, 2024

I agree. This PR is more about a demonstration for the impact of scalafmt 3.8, which is no hurry to be merged for now.

@bowenliang123 bowenliang123 changed the title Change importSelectors to binpack style with Scalafmt bumped to 3.8 Change importSelectors to binpack style with Scalafmt 3.8 Sep 6, 2024
@bowenliang123 bowenliang123 force-pushed the scalafmt38 branch 2 times, most recently from 07eb6eb to c50db64 Compare October 16, 2024 01:22
Copy link

github-actions bot commented Feb 3, 2025

Thanks for the PR! This PR is being closed due to inactivity. This isn't a judgement on the merit of the PR in any way. If this is still an issue with the latest version of Kyuubi, please reopen it and ask a committer to remove the Stale tag!

Thank you for using Kyuubi!

@bowenliang123
Copy link
Contributor Author

Hi @pan3793 , I've reopened this PR and it should be ready for merge.
After further investigation latest changes in Scalafmt's configs, singleLine for binPack.importSelectors helps minimizing the impacts to our code, while the fold option would be a better option to conforming max columns. Config doc: binPack.importSelectors

@bowenliang123 bowenliang123 changed the title Change importSelectors to binpack style with Scalafmt 3.8 Bump Scalafmt to 3.8.x Feb 3, 2025
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants