Skip to content

Conversation

@Kludex
Copy link
Member

@Kludex Kludex commented Nov 18, 2024

@Kludex Kludex requested a review from alexmojaki November 18, 2024 08:08
@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Nov 18, 2024

Deploying logfire-docs with  Cloudflare Pages  Cloudflare Pages

Latest commit: 1fd9012
Status: ✅  Deploy successful!
Preview URL: https://20ab6e74.logfire-docs.pages.dev
Branch Preview URL: https://support-instrument-parametre.logfire-docs.pages.dev

View logs

@codecov
Copy link

codecov bot commented Nov 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (267c83a) to head (1fd9012).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #607   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          133       133           
  Lines        10541     10549    +8     
  Branches      1448      1450    +2     
=========================================
+ Hits         10541     10549    +8     

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


🚨 Try these New Features:

) -> Callable[[Callable[P, R]], Callable[P, R]]: ...

@overload
def instrument(self, msg_template: Callable[P, R]) -> Callable[P, R]: ...
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
def instrument(self, msg_template: Callable[P, R]) -> Callable[P, R]: ...
def instrument(self, func: Callable[P, R]) -> Callable[P, R]: ...

Copy link
Member Author

Choose a reason for hiding this comment

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

This is not possible. The parameter names must match.


def instrument(
self,
msg_template: Callable[P, R] | LiteralString | None = None,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
msg_template: Callable[P, R] | LiteralString | None = None,
msg_template_or_func: Callable[P, R] | LiteralString | None = None,

@Kludex
Copy link
Member Author

Kludex commented Nov 18, 2024

I didn't do what the suggestions suggest because it's a breaking change... People could be doing msg_template=....

Or do you want to add the /?

@alexmojaki
Copy link
Contributor

OK leave the main signature as is but use the name func in the overload and silence pyright about it.

@Kludex
Copy link
Member Author

Kludex commented Nov 18, 2024

OK leave the main signature as is but use the name func in the overload and silence pyright about it.

The problem is the typing issue is not on the overload method, but on the main function itself... i.e. I'll have to type: ignore the main function.

Screenshot 2024-11-18 at 10 56 28

We may make typing mistakes in the instrument without noticing if we do this.

@alexmojaki
Copy link
Contributor

What if you just ignore that specific error code?

@Kludex
Copy link
Member Author

Kludex commented Nov 18, 2024

What if you just ignore that specific error code?

I'm not sure if this is working as expected, but done.

Co-authored-by: Marcelo Trylesinski <[email protected]>
@Kludex Kludex merged commit 641581c into main Nov 18, 2024
15 checks passed
@Kludex Kludex deleted the support-instrument-parametreless branch November 18, 2024 15:43
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.

@logfire.instrument should work in bear mode

3 participants