fix #1464: если в атрибуте не указан алиас, то им будет имя метода #1466
Conversation
WalkthroughThe changes in this pull request involve modifications to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ClassBuilder
participant ContextMethodAttribute
participant ContextMethodMapper
User->>ClassBuilder: Call ExportClassMethod(nativeMethod)
ClassBuilder->>ContextMethodAttribute: Create instance with nativeMethod.Name, null
ContextMethodAttribute-->>ClassBuilder: Return instance
ClassBuilder-->>User: Return method export result
User->>ContextMethodMapper: Call CreateMetadata()
ContextMethodMapper->>ContextMethodAttribute: Call GetAlias()
ContextMethodAttribute-->>ContextMethodMapper: Return alias
ContextMethodMapper-->>User: Return metadata
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/OneScript.Core/Contexts/ContextMethodAttribute.cs (2)
20-30: Consider enhancing error messages with more context.The validation logic and immutability are well implemented. However, the error messages could be even more helpful.
Consider this enhancement:
- throw new ArgumentException($"Name '{name}' must be a valid identifier"); + throw new ArgumentException($"Name '{name}' must be a valid identifier", nameof(name)); - throw new ArgumentException($"Alias '{alias}' must be a valid identifier"); + throw new ArgumentException($"Alias '{alias}' must be a valid identifier", nameof(alias));This change adds the parameter name to the exception, which helps with debugging and makes the error origin clearer.
32-36: Add XML documentation for better API understanding.The constructor implementation is clever and aligns with the PR objective. However, it would benefit from documentation explaining its purpose.
Consider adding XML documentation:
+ /// <summary> + /// Creates a new context method attribute with an automatically derived alias from the native method name. + /// </summary> + /// <param name="name">The name of the context method</param> + /// <param name="_">Ignored parameter to distinguish from the primary constructor</param> + /// <param name="nativeMethodName">Automatically populated with the declaring method's name</param> public ContextMethodAttribute(string name, string _ = null, [CallerMemberName] string nativeMethodName = null) : this(name, nativeMethodName) { }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- src/OneScript.Core/Contexts/ClassBuilder.cs (1 hunks)
- src/OneScript.Core/Contexts/ContextMethodAttribute.cs (2 hunks)
- src/ScriptEngine/Machine/Contexts/ContextMethodMapper.cs (1 hunks)
🔇 Additional comments (4)
src/OneScript.Core/Contexts/ContextMethodAttribute.cs (2)
9-9: LGTM!The addition of System.Runtime.CompilerServices is necessary for the CallerMemberName attribute usage.
38-40: Verify removal of deprecated state handling.The simplified getters improve readability, but let's verify that the removal of deprecated state handling doesn't break existing functionality.
src/OneScript.Core/Contexts/ClassBuilder.cs (1)
69-69: LGTM! The change implements the PR objective.The modification correctly implements the requirement to use method name as alias when no custom alias is specified, by passing the method name as first parameter and explicitly indicating no custom alias with null.
Let's verify that the ContextMethodAttribute constructor accepts null for the alias parameter:
✅ Verification successful
Constructor accepts null alias parameter, change is safe
Looking at the
ContextMethodAttributeconstructor implementation insrc/OneScript.Core/Contexts/ContextMethodAttribute.cs, it explicitly handles the case of null/empty alias through the conditionif (!string.IsNullOrEmpty(alias)). The validation is only performed when alias has a value, making it safe to pass null.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for ContextMethodAttribute constructor definition ast-grep --pattern 'class ContextMethodAttribute { $$$ public ContextMethodAttribute($_, $_) { $$$ } $$$ }'Length of output: 121
Script:
#!/bin/bash # Let's try a different approach to find ContextMethodAttribute constructor # First, let's find the file containing ContextMethodAttribute fd "ContextMethodAttribute" --type f # Then use ripgrep to search for constructor definition with context rg "ContextMethodAttribute\(.*\)" -A 5Length of output: 1237
src/ScriptEngine/Machine/Contexts/ContextMethodMapper.cs (1)
166-166: LGTM! The change aligns with PR objective.The modification to use
binding.GetAlias()without parameters aligns with the PR objective of using method names as default aliases. This change suggests that the alias handling logic has been centralized in theGetAlias()method.Let's verify the related changes in ContextMethodAttribute:
|
Добрый день!
В период с 27.10.2024 по 10.11.2024 включительно, буду находиться в ежегодном отпуске.
Почта просматриваться не будет.
|
Summary by CodeRabbit
New Features
ContextMethodAttributeclass constructor for invalid identifiers.ContextMethodAttributefor improved flexibility.Bug Fixes
CreateMetadatamethod for better accuracy.Refactor
ContextMethodAttributefor cleaner code.