-
Notifications
You must be signed in to change notification settings - Fork 3
nil dereferencing bug fixed #116
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
Conversation
|
@volodya-lombrozo please, check |
volodya-lombrozo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yegor256 Great thanks for the contribution! Just a single suggestion.
| } | ||
|
|
||
| func (a *Artifacts) Marshal() *protocol.MessageSendParams { | ||
| if a == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yegor256 I don't think we need to allow a nil receiver here. By doing this, we’re just hiding the original problem: 'Why do we have a nil here at all?'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@volodya-lombrozo I have no idea, to be honest...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yegor256 can we just remove it then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@volodya-lombrozo I can try, but the bug may not be fixed in this case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yegor256 Maybe you can try it locally?
go install
refrax ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@volodya-lombrozo tests fail if I remove this check for nil. I can simply drop this PR, but I don't know how to fix the bug otherwise (since this PR is made by Claude Code, not me)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yegor256 I suggest closing it. I've successfully reproduced the issue. Thus, I can fix the bug properly
fixes #115