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

Native targets for Functional and Logging #26

Merged
merged 6 commits into from
Dec 8, 2020

Conversation

cedrickcooke
Copy link
Contributor

Functional

No changes, everything just worked.

Logging

No StackTraceTagGenerator for now, not ruling it out for later. Hit two snags in my first stab:

  • Instantiating a Throwable was setting off the memory-leak crash. I think it was a false positive, but didn't want to spend too much time debugging, because...
  • Stack traces in Release builds are super useless without the mapping file, which wouldn't be available at runtime.

No console logger tests for now. In theory it should be possible to freopen the streams for stdout and stderr, but I had trouble getting that working.

  • Verified instead by making a sample app that used them.
  • Looks like we don't run tests for OSX targets anyways in CI?

@cedrickcooke cedrickcooke requested review from a team, twyatt and sdonn3 December 8, 2020 01:59
@twyatt
Copy link
Member

twyatt commented Dec 8, 2020

Looks like we don't run tests for OSX targets anyways in CI?

I'd love to have this setup, just haven't gotten around to it, since it requires using a macOS machine on CI. Created #27 to track.

logging/src/commonMain/kotlin/DispatchLogger.kt Outdated Show resolved Hide resolved
logging/src/commonMain/kotlin/Log.kt Outdated Show resolved Hide resolved
@cedrickcooke
Copy link
Contributor Author

Honestly I started with Stately out of fear, but AtomicFU turned out to be much easier to work with.

@cedrickcooke cedrickcooke requested a review from twyatt December 8, 2020 20:21
@codecov
Copy link

codecov bot commented Dec 8, 2020

Codecov Report

Merging #26 (72534c6) into main (7d5aee0) will increase coverage by 0.12%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main      #26      +/-   ##
============================================
+ Coverage     92.85%   92.98%   +0.12%     
- Complexity       57       58       +1     
============================================
  Files            12       12              
  Lines           112      114       +2     
  Branches         12       12              
============================================
+ Hits            104      106       +2     
  Misses            8        8              
Impacted Files Coverage Δ Complexity Δ
logging/src/commonMain/kotlin/DispatchLogger.kt 100.00% <100.00%> (ø) 11.00 <10.00> (ø)
logging/src/commonMain/kotlin/Log.kt 100.00% <100.00%> (ø) 28.00 <2.00> (+1.00)

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 7d5aee0...72534c6. Read the comment docs.

Copy link
Member

@twyatt twyatt left a comment

Choose a reason for hiding this comment

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

Wow, ya, atomicfu worked out great!

Comment on lines +47 to +49
public synthetic class com/juul/tuulbox/logging/LogAtomicTagGeneratorRefVolatile {
public fun <init> (Ljava/lang/Object;)V
}
Copy link
Member

Choose a reason for hiding this comment

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

Can't quite pin down where this is coming from (and if it needs to be on the public API)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's auto-generated from atomicfu's plugin. Not totally sure why

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 think the synthetic marker prevents library consumers from accessing it directly?

Copy link
Member

Choose a reason for hiding this comment

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

It's auto-generated from atomicfu's plugin. Not totally sure why

Oh gotchya. If it's generated from atomicfu, not much we can do (seems fine) — I was just a little confused where it was coming from.

I think the synthetic marker prevents library consumers from accessing it directly?

Not sure actually.

Copy link
Member

@twyatt twyatt left a comment

Choose a reason for hiding this comment

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

🍎 🚀 💯

Copy link
Contributor

@sdonn3 sdonn3 left a comment

Choose a reason for hiding this comment

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

had to do some reading up on atomicFU for learning purposes

@twyatt twyatt added this to the 2.3.1 milestone Dec 8, 2020
@cedrickcooke cedrickcooke merged commit 87a24e0 into main Dec 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants