rubocop-yard: Add support for string literals in TypesExplainer#1628
Conversation
| METHODMATCH = /(?:(?:#{NAMESPACEMATCH}|[a-z]\w*)\s*(?:#{CSEPQ}|#{NSEPQ})\s*)?#{METHODNAMEMATCH}/ | ||
|
|
||
| # Regular expression to match symbol and string literals | ||
| LITERALMATCH = /:\w+|'[^']*'|"[^"]*"/ |
There was a problem hiding this comment.
Does this constant make sense here?
There was a problem hiding this comment.
This regex seems a little overgreedy / not fully accurate with respect to string tokens. I don't think this covers the full case of valid symbol / string syntaxes.
There was a problem hiding this comment.
Moved it inside the TypesExplainer class.
lekemula
force-pushed
the
parse-string-and-symbol-literals
branch
3 times, most recently
from
1250332 to
2d79fb8
Compare
lekemula
changed the title
lsegal
left a comment
lsegal
left a comment
There was a problem hiding this comment.
I appreciate the attempt but the LITERALMATCH that this is based on seems perilous if used elsewhere.
We have another competing implementation in #1622 that also doesn't quite get this right but at least is less invasive (it's really just a naming change).
I think it would make more sense to pull that identifier regex into the types_explainer code directly so that the "best case" logic doesn't leak into other more generalized YARD code paths. My worry is that someone would see LITERALMATCH and use it for general code parsing, which would fail quickly. It's fine in the context of type specifier text, but not for blocks of code.
tldr LITERALMATCH is a case of premature generalization and that regex should be specific to the types explainer code, maybe not even a constant at all.
| METHODMATCH = /(?:(?:#{NAMESPACEMATCH}|[a-z]\w*)\s*(?:#{CSEPQ}|#{NSEPQ})\s*)?#{METHODNAMEMATCH}/ | ||
|
|
||
| # Regular expression to match symbol and string literals | ||
| LITERALMATCH = /:\w+|'[^']*'|"[^"]*"/ |
There was a problem hiding this comment.
This regex seems a little overgreedy / not fully accurate with respect to string tokens. I don't think this covers the full case of valid symbol / string syntaxes.
lekemula
force-pushed
the
parse-string-and-symbol-literals
branch
from
2d79fb8 to
861ea45
Compare
lekemula
force-pushed
the
parse-string-and-symbol-literals
branch
2 times, most recently
from
fd9eadd to
871c974
Compare
|
Thanks for the elaboration @lsegal - makes total sense.
While this PR does go a bit into refactoring, these changes make room for more complex changes from the following PR: #1630. I just wanted to extract the most out of it, to keep it as simple as possible. Let me know if you'd still want me to move the refactoring there. Additionally, #1622 seems to only handle the symbol literals, but not the string ones. That said, I'm totally fine with having it merged first and later extending it for the strings. |
|
We went the route of pulling #1622 because there was less blast radius. If you want to roll your changes over that, that would be great, but I think this needs a rebase etc. Thanks! |
|
Thanks for the notice @lsegal - will do! |
- Add LITERALMATCH regex constant to match symbol (:symbol) and string ('string', "string") literals
- Update TypesExplainer parser to recognize symbol and string literal types
- Format literal values as "a literal value :symbol" for better readability
- Add comprehensive test coverage for LITERALMATCH constant and literal type parsing
- Fixes parsing inconsistency between YARD TypesExplainer and online parser
Resolves lsegal#1627
🤖 Generated with [Claude Code](https://claude.ai/code)
Co-Authored-By: Claude <noreply@anthropic.com>
This could be useful for solargraph gem
lekemula
force-pushed
the
parse-string-and-symbol-literals
branch
from
871c974 to
6e204eb
Compare
lekemula
changed the title
lekemula
changed the title
|
Hi @lsegal, I rebased my changes, so this PR is ready for re-review 🙏 |
|
Thank you for this! |
…1630) # Description Follow-up on #1628 (omit first 2 commits when reviewing) Adds support for multiple and nested hash keys - example from specs: ``` ruby "Hash{:key_one, :key_two => String; :key_three => Symbol; :key_four => Hash{:sub_key_one => String}}" => "a Hash with keys made of (a literal value :key_one or a literal value :key_two) and values of (Strings) and keys made of (a literal value :key_three) and values of (Symbols) and keys made of (a literal value :key_four) and values of (a Hash with keys made of (a literal value :sub_key_one) and values of (Strings))", "Hash{:key_one => String, Number; :key_two => String}" => "a Hash with keys made of (a literal value :key_one) and values of (Strings or Numbers) and keys made of (a literal value :key_two) and values of (Strings)" ``` NOTE: These changes probably only impact on https://github.com/ksss/rubocop-yard and have no effect elsewhere. PS. I think this alongside the base PR, will open up a lot of opportunities for better `Hash` completion/typechecking for [solargraph](https://github.com/castwide/solargraph) gem. # Completed Tasks - [x] I have read the [Contributing Guide][contrib]. - [x] The pull request is complete (implemented / written). - [x] Git commits have been cleaned up (squash WIP / revert commits). - [x] I wrote tests and ran `bundle exec rake` locally (if code is attached to PR). [contrib]: https://github.com/lsegal/yard/blob/main/CONTRIBUTING.md --------- Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Loren Segal <lsegal@soen.ca>
2 participants
Description
Resolves #1627
🤖 Generated with Claude Code
Completed Tasks
bundle exec rakelocally (if code is attached to PR).