Skip to content
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

util/util-logging: Add Scala3 cross-build #295

Closed
wants to merge 4 commits into from

Conversation

felixbr
Copy link
Contributor

@felixbr felixbr commented Sep 9, 2021

This adds Scala3 cross-buliding for the module util-logging as discussed in #290

@@ -285,7 +285,7 @@ class FileHandlerTest extends AnyWordSpec with TempFolder {

val handler = FileHandler(
filename = folderName + "/test.log",
rollPolicy = Policy.MaxSize(maxSize.bytes),
rollPolicy = Policy.MaxSize(maxSize.toLong.bytes),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This conversion is necessary in Scala3 because the extension method is only defined on Long, and it no longer implicitly converts to Int. In #290 I added an implicit conversion as forwarder but that's not landed in develop yet.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we strip out the forwarders into a separate PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can make a PR based on what I've already added in #290 to keep source compatibility for Scala3

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The separate PR is here: #296

@codecov-commenter
Copy link

codecov-commenter commented Sep 9, 2021

Codecov Report

Merging #295 (23767e6) into develop (91551dc) will not change coverage.
The diff coverage is 80.00%.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop     #295   +/-   ##
========================================
  Coverage    52.62%   52.62%           
========================================
  Files          316      316           
  Lines        16864    16864           
  Branches      1035     1045   +10     
========================================
  Hits          8875     8875           
  Misses        7989     7989           
Impacted Files Coverage Δ
...g/src/main/scala/com/twitter/logging/Handler.scala 47.05% <0.00%> (ø)
...c/main/scala/com/twitter/logging/FileHandler.scala 82.97% <100.00%> (ø)
...ng/src/main/scala/com/twitter/logging/Logger.scala 61.29% <100.00%> (ø)
...in/scala/com/twitter/logging/QueueingHandler.scala 93.75% <0.00%> (-6.25%) ⬇️
util-core/src/main/scala/com/twitter/io/Buf.scala 93.27% <0.00%> (+0.35%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 91551dc...23767e6. Read the comment docs.

@felixbr
Copy link
Contributor Author

felixbr commented Sep 9, 2021

Not sure why com.twitter.app.AppTest is failing (different module and works locally), might be a flaky test.

Copy link
Contributor

@mosesn mosesn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks great, thanks so much @felixbr!

finaglehelper pushed a commit that referenced this pull request Oct 13, 2021
This fixes lost source-compatibility for unit conversions from Int values in
Scala3. As discussed in #295 I extracted/adapted this from #290 (previous
discussion).

Previously something like 1.toInt.second did compile in Scala2 (with default
settings) but didn't in Scala3.

I added forwarders for all units which use Long as base, as that seemed
reasonable and safe. I didn't add an implicit conversion from Float to Double
because floating-point types are dumb and I don't trust the conversion to be
lossless in all cases.

Signed-off-by: Yufan Gong <yufang@twitter.com>

Differential Revision: https://phabricator.twitter.biz/D759923
@felixbr
Copy link
Contributor Author

felixbr commented Oct 15, 2021

I rebased the PR onto develop, removed the now unnecessary .toLong conversion, and fixed one more deprecation warning regarding Scala 3.

I think the PR can now be merged, if you like. 🙂

finaglehelper pushed a commit that referenced this pull request Oct 18, 2021
Cross build util-logging with Scala 3. Util PR #295

Signed-off-by: Hamdi Allam <hallam@twitter.com>

JIRA Issues: CSL-11409

Differential Revision: https://phabricator.twitter.biz/D764508
@joybestourous
Copy link
Contributor

Merged! b8a30a7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants