-
-
Notifications
You must be signed in to change notification settings - Fork 745
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
feat: add new PathPrefix attribute #1685
Closed
JesseKlaasse
wants to merge
5
commits into
reactiveui:main
from
JesseKlaasse:feat/path-prefix-attribute
Closed
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
56e15e6
feat: add new PathPrefix attribute
JesseKlaasse da34ff8
Merge branch 'main' into feat/path-prefix-attribute
JesseKlaasse 2e87757
Merge branch 'main' into feat/path-prefix-attribute
ChrisPulman 1de9cae
test: add unit tests for ReflectionHelpers
JesseKlaasse a7ead77
Merge branch 'main' into feat/path-prefix-attribute
JesseKlaasse File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
using System; | ||
using System.Collections.Generic; | ||
using System.Threading.Tasks; | ||
using Refit; // InterfaceStubGenerator looks for this | ||
using Refit.Tests.SeparateNamespaceWithModel; | ||
|
||
using static System.Math; // This is here to verify https://github.com/reactiveui/refit/issues/283 | ||
|
||
namespace Refit.Tests | ||
{ | ||
[Headers("User-Agent: Refit Integration Tests")] | ||
[PathPrefix("/ping")] | ||
public interface IPathPrefix | ||
{ | ||
[Get("/get?result=Ping")] | ||
Task<string> Ping(); | ||
} | ||
|
||
[Headers("User-Agent: Refit Integration Tests")] | ||
public interface IInheritingPathPrefix : IPathPrefix | ||
{ | ||
[Get("/get?result=Pang")] | ||
Task<string> Pang(); | ||
} | ||
|
||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,54 @@ | ||
using System.Reflection; | ||
|
||
namespace Refit | ||
{ | ||
/// <summary> | ||
/// Provides utility methods for reflection-based operations. | ||
/// </summary> | ||
public static class ReflectionHelpers | ||
{ | ||
/// <summary> | ||
/// Retrieves the path prefix defined by a <see cref="PathPrefixAttribute"/> on a specified interface or its inherited interfaces. | ||
/// </summary> | ||
/// <param name="targetInterface">The interface type from which to retrieve the path prefix.</param> | ||
/// <returns> | ||
/// The path prefix if a <see cref="PathPrefixAttribute"/> is found; otherwise, an empty string. | ||
/// </returns> | ||
/// <remarks> | ||
/// This method first checks the specified interface for the <see cref="PathPrefixAttribute"/>. If not found, | ||
/// it then checks each interface inherited by the target interface. If no attribute is found after all checks, | ||
/// the method returns an empty string. | ||
/// </remarks> | ||
/// <exception cref="ArgumentNullException"> | ||
/// Thrown if <paramref name="targetInterface"/> is null. | ||
/// </exception> | ||
public static string GetPathPrefixFor(Type targetInterface) | ||
{ | ||
// Manual null check for compatibility with older .NET versions | ||
if (targetInterface == null) | ||
{ | ||
throw new ArgumentNullException(nameof(targetInterface)); | ||
} | ||
|
||
// Check if the attribute is applied to the type T itself | ||
var attribute = targetInterface.GetCustomAttribute<PathPrefixAttribute>(); | ||
if (attribute != null) | ||
{ | ||
return attribute.PathPrefix; | ||
} | ||
|
||
// If the attribute is not found on T, check its interfaces | ||
foreach (var interfaceType in targetInterface.GetInterfaces()) | ||
{ | ||
attribute = interfaceType.GetCustomAttribute<PathPrefixAttribute>(); | ||
if (attribute != null) | ||
{ | ||
return attribute.PathPrefix; | ||
} | ||
} | ||
|
||
// If the attribute is still not found, return empty string | ||
return string.Empty; | ||
} | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Is concatenation enough? I think it should also add a
/
separator if necessary.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.
I don't think Refit should try to be intelligent by adding slashes. I think it's the responsibility of the implementing user to annotate correctly. There could be situations where you actually want to concatenate instead of an automatically added slash. Don't you think?
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.
To be honest, I can't really think of any situation where I would want to concatenate without the slash. Unless you want to have a prefix like "i" and suffixes like "tem" and "nventory", but that seems like a bad idea to me...
I think Refit should "do the right thing" to make things easier for the developer. But I guess the right answer is to check what Refit does elsewhere, and do the same...
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.
@thomaslevesque Agreed, on both points. I will take a look into that.
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.
https://github.com/reactiveui/refit/blob/main/Refit/RequestBuilderImplementation.cs#L639-L640
It seems like Refit is also just concatenating the basepath and the relative path (unless the basepath is empty). I agree that this could be more foolproof, however, it seems like a separate issue to 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.
Agreed. Let's leave it like this for now.