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

Add Effect.whenLogLevel #4342

Merged
merged 9 commits into from
Jan 30, 2025
Merged

Conversation

indietyp
Copy link

Type

  • Refactor
  • Feature
  • Bug Fix
  • Optimization
  • Documentation Update

Description

This is the result from the following Discord discussion: https://discord.com/channels/795981131316985866/1332442902629978132, it implements a new function Effect.whenLogLevel, for which the specified effect is only executed whenever the log level is enabled.

I am unsure if the return type here is okay, currently it permits the effect to return a value, and wrap the return value in Option.Option. The other possibility would be to simply say the return must be (like for Effect.tap) to be void. I could imagine scenarios in which it could be helpful to get the return value, but also feel like that it could be abused in a sense, 95% of usecases would likely just yield* without considering the return value (but one could for example use the return value to determine if other diagnostics should be generated, etc.)

Related

@indietyp indietyp requested a review from mikearnaldi as a code owner January 25, 2025 21:08
Copy link

changeset-bot bot commented Jan 25, 2025

🦋 Changeset detected

Latest commit: 8a1f47d

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 35 packages
Name Type
effect Minor
@effect/cli Major
@effect/cluster-browser Major
@effect/cluster-node Major
@effect/cluster-workflow Major
@effect/cluster Major
@effect/experimental Major
@effect/opentelemetry Major
@effect/platform-browser Major
@effect/platform-bun Major
@effect/platform-node-shared Major
@effect/platform-node Major
@effect/platform Major
@effect/printer-ansi Major
@effect/printer Major
@effect/rpc-http Major
@effect/rpc Major
@effect/sql-clickhouse Major
@effect/sql-d1 Major
@effect/sql-drizzle Major
@effect/sql-kysely Major
@effect/sql-libsql Major
@effect/sql-mssql Major
@effect/sql-mysql2 Major
@effect/sql-pg Major
@effect/sql-sqlite-bun Major
@effect/sql-sqlite-do Major
@effect/sql-sqlite-node Major
@effect/sql-sqlite-react-native Major
@effect/sql-sqlite-wasm Major
@effect/sql Major
@effect/typeclass Major
@effect/vitest Major
@effect/ai Major
@effect/ai-openai Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@gcanti gcanti requested a review from tim-smart January 30, 2025 12:08
<A, E, R>(effect: Effect.Effect<A, E, R>, level: LogLevel.LogLevel) => Effect.Effect<Option.Option<A>, E, R>
>(2, (effect, level) => {
return core.withFiberRuntime((fiberState) => {
const currentLogLevel = FiberRef.currentMinimumLogLevel.pipe(fiberState.getFiberRef)
Copy link
Contributor

Choose a reason for hiding this comment

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

No need for the .pipe here

Copy link
Author

Choose a reason for hiding this comment

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

fixed in 1dce660, I've also taken care of aligning the code used to filter with the one used in FiberRuntime.log just to keep it consistent (e.g. moving from LogLevel.lessThanEqual to LogLevel.greaterThan and doing an early return.

@@ -948,6 +948,20 @@ export const logAnnotations: Effect.Effect<HashMap.HashMap<string, unknown>> = c
core.currentLogAnnotations
)

/** @internal */
export const whenLogLevel = dual<
(level: LogLevel.LogLevel) => <A, E, R>(effect: Effect.Effect<A, E, R>) => Effect.Effect<Option.Option<A>, E, R>,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should support passing a string literal too.

Copy link
Author

Choose a reason for hiding this comment

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

added in 1dce660

@effect-bot effect-bot changed the base branch from main to next-minor January 30, 2025 18:56
@indietyp indietyp requested a review from tim-smart January 30, 2025 18:57
@indietyp indietyp requested a review from tim-smart January 30, 2025 19:15
@tim-smart
Copy link
Contributor

Hmm will need to move the implementation into the fiberRuntime file to prevent the circular dependency.

@indietyp
Copy link
Author

Hmm will need to move the implementation into the fiberRuntime file to prevent the circular dependency.

this should be resolved in 474f51c. I've also fixed the external API to allow for literals in 8a1f47d.

Copy link
Contributor

@tim-smart tim-smart left a comment

Choose a reason for hiding this comment

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

🚀

@tim-smart tim-smart merged commit e1b3701 into Effect-TS:next-minor Jan 30, 2025
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants