First PR
This blog is about the two PRs I made this week. If you’ve read my last blog about my contribution to ChatCraft, where I worked on the Ask and Retry menus, you’ll know that I contributed a reusable component for those menus. While working on that issue, I discovered a bug.
The bug occurred when you retried a response—sometimes the request failed on the LLM side, but no error message was displayed on ChatCraft when it happened. To address this, I opened an issue, included a video demonstrating the bug, and asked the maintainer if I could work on it.
Sometime AI message retry is failing.
#716
https://github.com/user-attachments/assets/769179fd-2428-4725-8ec9-3fe759ea887e
Hi @humphd @tarasglek , I notice that some AI message retry is failing API call.
After receiving the maintainer's approval to work on this, I started investigating the issue. My first task was to locate the function responsible for making the API call for the retry action. I began by examining the component that renders the retry button. Then, I followed the chain of parent components that passed the RetryFunction to this component. Eventually, I found the function handleRetryClick, which contained the logic for handling retries. Interestingly, there was a //TODO comment left by the original developer to add UI error handling in the catch block.
After identifying the location, my next task was to display the error message using the useAlert() hook, as requested by the maintainer. I implemented the change by adding the error() method like this:
I then created a PR for this change and requested the maintainer's review.
added UI Error display for failed retry on message
#733
This Fixes #716
- chatcraft.org\src\components\Message\AiMessage.tsx (https://github.com/tarasglek/chatcraft.org/blob/83d180244708becc62ac61e4452b923a2412a33b/src/components/Message/AiMessage.tsx#L130)
Update the catch block for retry from this
catch (err) {
// TODO: UI error handling
console.warn("Unable to retry message", { model, err });
}
to this
catch (err: any) {
error({
title: `Response Error`,
message: err.message,
});
console.warn("Unable to retry message", { model, err });
}
Here is the demo:
https://github.com/user-attachments/assets/371ebd0d-dbb1-4236-a38f-110eedee883e
The maintainer requested a small update to change the error message from Response Error to Retry Error. After making this change, my PR was merged.
Second PR: Improving Documentation for Member References in xUnit Attributes
Recently, I have been learning xUnit for writing unit test cases for one of my projects. While exploring the xUnit GitHub repository, I noticed it is an open-source project. This sparked my interest, and I started looking for issues to contribute to. I found a relatively simple issue:
Add an analyzer which recommends nameof usage
#2991
Whenever we use string properties on attributes intended to point at a member, we should encourage the use of nameof rather than hard-coded strings.
This includes, at least (please look and see if there are others):
IFactAttribute.SkipUnlessIFactAttribute.SkipWhen-
MemberDataAttributeconstructor
This issue aimed to improve the clarity and reliability of attributes that reference class members. The goal was to encourage developers to use safer practices, such as the nameof operator, when referencing members in attributes like MemberData, SkipWhen, and SkipUnless.
The Problem
When working with attributes like MemberDataAttribute, developers often rely on hard-coded strings to reference class members. For example:
[MemberData("SomeMemberName")]
This approach has several drawbacks:
- Refactoring Risks: If the member name changes, the string reference doesn’t update, leading to runtime errors.
-
Readability: Hard-coded strings are less clear than using
nameof. - Compile-Time Safety: Strings aren’t validated at compile time, making it easier to introduce typos or errors.
The Solution
To address this, I proposed an improvement to the XML documentation for these attributes. The updated documentation explicitly recommends using the nameof operator to reference members, ensuring compile-time safety and better readability.
Example Documentation Updates
For MemberDataAttribute:
/// <param name="memberName">
/// The name of the public static member on the test class that will provide the test data.
/// It is recommended to use the <c>nameof</c> operator to ensure compile-time safety,
/// e.g., <c>nameof(SomeMemberName)</c>.
/// </param>
For SkipWhen:
/// <remarks>
/// To avoid issues during refactoring, it is recommended to use the <c>nameof</c> operator
/// to reference the property, e.g., <c>SkipWhen = nameof(IsTestSkipped)</c>.
/// </remarks>
For SkipUnless:
/// <remarks>
/// The name of the public static member on the test class that will provide the test data.
/// It is recommended to use the <c>nameof</c> operator to ensure compile-time safety,
/// e.g., <c>nameof(SomeMemberName)</c>.
/// </remarks>
The Impact
These changes improve the developer experience by:
- Encouraging compile-time safety, reducing runtime errors.
- Enhancing readability and maintainability of code.
- Supporting better practices for refactoring, especially in large projects.
My PR
After implementing the changes, I submitted a PR and requested the maintainer’s review. You can find the PR here:
added recommendation to use nameof
#3062
This fixes #2991
-
MemberDataAttribute: Updated XML documentation for the
memberNameparameter to recommend using thenameofoperator for compile-time safety. -
SkipWhen: Updated XML documentation to suggest the use of
nameoffor referencing public static properties to ensure safer and more maintainable code. -
SkipUnless: Added a recommendation in the XML documentation to use the
nameofoperator when referencing public static properties, promoting best practices.
Reflections
This PR reinforced the importance of clear and detailed documentation in guiding developers toward best practices. While the change may seem small, it can significantly impact code quality and developer productivity.
Top comments (0)