Skip to content

Add namespace and improve ProgressToken null handling #388

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

Closed
wants to merge 2 commits into from

Conversation

Kamyab7
Copy link

@Kamyab7 Kamyab7 commented May 6, 2025

Added namespace declaration for EverythingServer and improved ProgressToken null handling to enhance code organization and prevent potential null reference issues.

Motivation and Context

  1. The SubscriptionMessageSender class was missing a namespace declaration, which could lead to organization and accessibility issues.
  2. The ProgressToken.ToString() method was returning null in certain cases, which could cause null reference exceptions in consuming code. This change ensures consistent behavior by returning an empty string instead.

How Has This Been Tested?

  • Verified that the namespace addition resolves compilation issues in the EverythingServer project
  • Tested ProgressToken.ToString() with various input types (string, long, and null) to ensure proper string conversion
  • Confirmed that existing functionality remains intact while improving null safety

Breaking Changes

No breaking changes. The modifications are backward compatible:

  • Namespace addition only affects internal organization
  • Empty string return instead of null is a safer alternative that maintains the same contract

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

Additional context

The changes focus on improving code quality and safety:

  1. The namespace addition follows C# conventions for code organization
  2. The empty string return in ProgressToken.ToString() follows the principle of least surprise and prevents null reference exceptions
  3. Both changes are minimal and focused, making them easy to review and maintain

@stephentoub
Copy link
Contributor

which could cause null reference exceptions in consuming code

In what code?

@Kamyab7
Copy link
Author

Kamyab7 commented May 6, 2025

which could cause null reference exceptions in consuming code

In what code?

I haven't found any actual issues in the current codebase, but I was being proactive about preventing potential null reference exceptions if someone misuses the method in the future. It's more of a defensive programming approach rather than fixing an existing bug.

@halter73
Copy link
Contributor

Thanks for your time on this. If you run into any real circumstances where you get null when you shouldn't, please let us no. Otherwise, we don't generally defend against things we know will not happen. We might add asserts if it improves clarity, but I don't think that's the case here. If a ProgressToken hasn't been initialized by its constructor, I think it's probably good that ToString() returns null. It shows something is awry.

@halter73 halter73 closed this May 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants