Migrate classes to new package structure by janstenpickle · Pull Request #748 · trace4cats/trace4cats

janstenpickle · 2022-04-30T15:49:32Z

Addresses #744

modules/tail-sampling/src/main/scala/trace4cats/samplling/tail/RateTailSpanSampler.scala

modules/meta/src/test/scala/io/janstenpickle/trace4cats/meta/TracedSpanExporterSpec.scala

catostrophe · 2022-05-15T07:11:29Z

@janstenpickle do you need help with this PR?

janstenpickle · 2022-05-17T08:01:06Z

@janstenpickle do you need help with this PR?

Sorry, I've been really busy the last couple of week. Yes please feel free to commit to this branch if you have some time!

janstenpickle · 2022-05-20T14:02:58Z

Hopefully I've got everything now 😅

janstenpickle · 2022-05-22T12:24:56Z

@bplommer how does this new structure look to you?

catostrophe · 2022-05-22T13:52:49Z

Will review tonight

modules/context-utils-laws/src/main/scala/trace4cats/context/laws/discipline/AskTests.scala

modules/core/src/main/scala/trace4cats/package.scala

bplommer · 2022-05-23T09:51:10Z

modules/core/src/main/scala/trace4cats/package.scala

+package object trace4cats {
+  type SpanName = String
+  type SpanParams = (SpanName, SpanKind, TraceHeaders, ErrorHandler)
+  type ResourceKleisli[F[_], -In, +Out] = Kleisli[Resource[F, +*], In, Out @uncheckedVariance]


I was going to suggest renaming ResourceKleisli to something that reflects purpose rather than implementation, and possibly changing it from a type alias to part of a class heirarchy with EntryPoint. But again, not something that should happen in this PR - just mentioning it while I remember

The main reason for not making this its own class is that we get a lot of things for free with Kleisli and its input and output aren't fixed to the input/output of the entry point. This allows us to compose the entrypoint with other means of establishing some context from some input. There's an example of this in the http4s library.

bplommer · 2022-05-23T09:54:40Z

Looks good! I wonder if types from model should also be aliased in the main package object?

janstenpickle · 2022-05-23T10:12:42Z

Looks good! I wonder if types from model should also be aliased in the main package object?

Yes, that might be handy for things like the IDs and context, I'll take a look.

janstenpickle · 2022-05-26T08:11:40Z

@bplommer @catostrophe how does this look to you both now, shall I merge?

I've got a scalafix branch in progress at the moment as well, so once the other repos are done we should be getting close to a release

catostrophe · 2022-05-26T08:13:14Z

Sorry for being so slow, been busy at $WORK. Checking now...

janstenpickle · 2022-05-26T08:14:15Z

Sorry for being so slow, been busy at $WORK. Checking now...

No problem, I know the feeling!

catostrophe

Why trace4catstests?

catostrophe · 2022-05-26T18:17:46Z

.../context-utils-laws/src/main/scala/trace4catstests/context/laws/discipline/UnliftTests.scala

-package io.janstenpickle.trace4cats.base.context
-package laws
-package discipline
+package trace4catstests.context.laws.discipline


Is it intended? 🤔

catostrophe · 2022-05-26T18:18:08Z

modules/context-utils-laws/src/test/scala/trace4cats/context/laws/IdTests.scala

+import cats.~>
 import org.scalacheck.Cogen
+import trace4cats.context.Unlift
+import trace4catstests.context.laws.discipline.UnliftTests


catostrophe · 2022-05-26T18:21:47Z

modules/iolocal/src/test/scala/trace4cats/context/iolocal/laws/IOLocalTests.scala

+import trace4cats.context.iolocal.GenIO
+import trace4cats.context.iolocal.instances.ioLocalProvide
+import trace4cats.context.laws.BaseSuite
+import trace4catstests.context.laws.discipline.ProvideTests


janstenpickle · 2022-05-27T10:01:45Z

Why trace4catstests?

@catostrophe - It was suggested by @bplommer. Sorry, I resolved the comment so you wouldn't have seen it!

catostrophe · 2022-05-27T11:05:22Z

Why trace4catstests?

@catostrophe - It was suggested by @bplommer. Sorry, I resolved the comment so you wouldn't have seen it!

I still haven't got the idea behind it.

I've never seen such a separation before in the context of discipline and law tests. Instead, laws and tests are usually in the same namespace and are close enough, because, if they are ever edited, they are edited together.

See:
https://github.com/typelevel/cats/tree/main/kernel-laws/shared/src/main/scala/cats/kernel/laws/discipline
https://github.com/typelevel/cats-tagless/tree/master/laws/src/main/scala/cats/tagless/laws/discipline
https://github.com/typelevel/cats-effect/tree/series/3.x/laws/shared/src/main/scala/cats/effect/laws

catostrophe · 2022-05-27T11:07:55Z

These "tests" are actually rule sets that you use in real tests to check some concrete implementation for satisfying them.

janstenpickle · 2022-05-27T13:15:15Z

I've never seen such a separation before in the context of discipline and law tests. Instead, laws and tests are usually in the same namespace and are close enough, because, if they are ever edited, they are edited together.

I've got to admit I took the recommendation at face value. I wonder if you were thinking more of the separate test modules rather than discipline tests @bplommer?

bplommer · 2022-05-27T13:31:43Z

I've got to admit I took the recommendation at face value. I wonder if you were thinking more of the separate test modules rather than discipline tests @bplommer?

Yes, afraid I didn't look very closely before suggesting that. Anything that's going to be published as an artifact, I'd leave in the trace4cats package.

catostrophe · 2022-05-27T14:03:42Z

Could we revert the introduction of trace4catstests package?

The rest is fine 👍

janstenpickle · 2022-05-28T13:09:24Z

Could we revert the introduction of trace4catstests package?

The rest is fine 👍

Cool, I'll revert this now then merge. Thanks all!

janstenpickle added 6 commits April 30, 2022 15:43

Move context utils to new package

fded057

Move kernel classes to new package

1c51df6

Alias kernel types

e1ae301

Move core types to new package

5a05065

Move testkit to new package

e0f8731

Move tail sampling to new package

08853ac

catostrophe requested review from catostrophe and ybasket April 30, 2022 17:31

catostrophe reviewed Apr 30, 2022

View reviewed changes

modules/tail-sampling/src/main/scala/trace4cats/samplling/tail/RateTailSpanSampler.scala Outdated Show resolved Hide resolved

catostrophe reviewed Apr 30, 2022

View reviewed changes

modules/meta/src/test/scala/io/janstenpickle/trace4cats/meta/TracedSpanExporterSpec.scala Show resolved Hide resolved

janstenpickle added 4 commits May 20, 2022 14:38

Fix package name typo

81b1688

Move meta to new package

970cd7b

Merge branch 'master' into module-reorg-part2

04b0c41

Fix 2.12 build

43b78e6

janstenpickle marked this pull request as ready for review May 20, 2022 14:02

janstenpickle requested a review from catostrophe May 20, 2022 14:03

janstenpickle added 2 commits May 22, 2022 13:04

Move fs2 to new package

734e2a0

Fix 2.12 compile error

efb9b7b

bplommer reviewed May 23, 2022

View reviewed changes

modules/context-utils-laws/src/main/scala/trace4cats/context/laws/discipline/AskTests.scala Show resolved Hide resolved

bplommer reviewed May 23, 2022

View reviewed changes

modules/core/src/main/scala/trace4cats/package.scala Show resolved Hide resolved

bplommer reviewed May 23, 2022

View reviewed changes

janstenpickle mentioned this pull request May 23, 2022

Remove SpanName type alias #757

Closed

Add aliases for model types

dc9e786

janstenpickle added 2 commits May 23, 2022 14:15

Change base package for context tests

0b30871

Merge branch 'master' into module-reorg-part2

be1fd55

catostrophe reviewed May 26, 2022

View reviewed changes

Revert base package for tests

31c9f23

janstenpickle merged commit 8ded233 into master May 28, 2022

janstenpickle deleted the module-reorg-part2 branch May 28, 2022 13:14

catostrophe added the enhancement New feature or request label May 29, 2022

Conversation

janstenpickle commented Apr 30, 2022

Uh oh!

Uh oh!

Uh oh!

catostrophe commented May 15, 2022

Uh oh!

janstenpickle commented May 17, 2022

Uh oh!

janstenpickle commented May 20, 2022

Uh oh!

janstenpickle commented May 22, 2022

Uh oh!

catostrophe commented May 22, 2022

Uh oh!

Uh oh!

Uh oh!

bplommer May 23, 2022

Choose a reason for hiding this comment

Uh oh!

janstenpickle May 23, 2022

Choose a reason for hiding this comment

Uh oh!

bplommer commented May 23, 2022

Uh oh!

janstenpickle commented May 23, 2022

Uh oh!

janstenpickle commented May 26, 2022

Uh oh!

catostrophe commented May 26, 2022

Uh oh!

janstenpickle commented May 26, 2022

Uh oh!

catostrophe left a comment

Choose a reason for hiding this comment

Uh oh!

catostrophe May 26, 2022

Choose a reason for hiding this comment

Uh oh!

catostrophe May 26, 2022

Choose a reason for hiding this comment

Uh oh!

catostrophe May 26, 2022

Choose a reason for hiding this comment

Uh oh!

janstenpickle commented May 27, 2022

Uh oh!

catostrophe commented May 27, 2022

Uh oh!

catostrophe commented May 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

janstenpickle commented May 27, 2022

Uh oh!

bplommer commented May 27, 2022

Uh oh!

catostrophe commented May 27, 2022

Uh oh!

janstenpickle commented May 28, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

catostrophe commented May 27, 2022 •

edited

Loading