Repository navigation
Conversation
kavix
changed the title
|
@kinto0 has imported this pull request. If you are a Meta employee, you can view this in D110947960. |
kinto0
left a comment
kinto0
left a comment
There was a problem hiding this comment.
hey, thanks for taking this on. left a few suggestions
This comment has been minimized.
This comment has been minimized.
kinto0
left a comment
kinto0
left a comment
There was a problem hiding this comment.
remaining questions about the logging calls
67ba85c to
5056bfd
Compare
|
thanks! looks good to me. then we will get another quick set of eyes on it and then we can merge! |
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
|
Hi @kinto0, could you please Re-review this PR ? Thanks! |
yangdanny97
left a comment
yangdanny97
left a comment
There was a problem hiding this comment.
Review automatically exported from Phabricator review in Meta.
kavix
left a comment
kavix
left a comment
There was a problem hiding this comment.
Great feedback — thanks for the detailed review.
I agree on both points and will follow up with
- Additional format-specifier coverage for %%, named + width/precision cases, and malformed/incomplete specifiers.
- Clear documentation of is_logger_candidate boundaries (expected false positives/false negatives), plus focused tests for those limits where practical.
kinto0
left a comment
kinto0
left a comment
There was a problem hiding this comment.
sorry, a few more changes were requested (I don't think you saw these yet)
|
This pull request has been automatically marked as stale because it has not had recent activity for more than 2 weeks. If you are still working on this this pull request, please add a comment or push new commits to keep it active. Otherwise, please unassign yourself and allow someone else to take over. Thank you for your contributions! |
c213f8a to
36dffcd
Compare
…3908) Why: Highlight printf-style format specifiers inside logging and formatting strings, similar to clangd in C/C++. Since Ruff flags Python logging f-strings for lazy evaluation performance reasons, users are encouraged to use percent-style formatting, which should be highlighted for readability. What: - Add `formatSpecifier` semantic token type to LSP legends. - Scan string literals passed to calls or as left operand of `%` operator for printf-style format specifiers using a regex. - Scan raw source slice directly to accurately map token byte offsets without character drift from escape sequences. - Consume `%%` as literal percent without emitting tokens. - Add comprehensive integration tests covering basic specifiers, escaped prefixes, `%%`, named arguments, width, precision, and string `%` modulo operations. Fixes meta#3908
36dffcd to
70cb635
Compare
|
Hi @kinto0 @yangdanny97 , could you please Re-review this PR ? Thanks! |
|
Closing this PR because linked issue #3908 was closed. |
|
hey, sorry about the delay and the back/forth. it actually looks like we already support these format specifiers syntactically in editors that combine our results with textmate grammars (see the issue). if it's absolutely necessary, we might add a textmate parser for all syntax, but until then, I don't think the pyrefly analysis should have this role. I'm sorry I marked it a good-first-issue. I now can see our struggles implementing it and realize it is difficult and out of scope for pyrefly. |
Summary
Fixes #3908
This PR adds a
formatSpecifiersemantic token for Python logging calls, similar to how clangd highlights format specifiers in C/C++.Since Ruff flags Python logging f-strings (for lazy evaluation performance reasons via rule G004), users are encouraged to use percent-style formatting. This change ensures that when users follow this best practice, their format specifiers (
%s,%d,%(name)s, etc.) are properly syntax-highlighted by the IDE for readability.Approach & Details
formatSpecifier: Added this custom token type to theSemanticTokensLegendsregistry.is_logger_candidate) to detect common logging functions (e.g.logger.info,logging.debug,LOGGER.warn) to avoid costly symbol resolutions on the hot path.OnceLockcached regex that strictly follows Python's printf-style format specifiers to scan string literals passed to the detected logging calls.process_astsemantic token builder pipeline to propagate thesourcetext string down through the visitor so that string literals can be sliced and matched against the regex.semantic_tokens_format_specifierintegration test tolsp_interactionwhich successfully asserts the LSP token mapping over the string"Hello %s %d".