Skip to content
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

Stub anything that looks like a Refit interface #67

Merged
merged 5 commits into from
Nov 3, 2014
Merged

Stub anything that looks like a Refit interface #67

merged 5 commits into from
Nov 3, 2014

Conversation

bennor
Copy link
Contributor

@bennor bennor commented Oct 25, 2014

This should resolve #65. InterfaceStubGenerator now looks for any interface in a file with using Refit; that has at least one method with a [Get|Head|Post|Put|Delete(.*)] attribute on it.

I have a couple of concerns about the approach, but I'll comment in-line so it makes more sense.

// find any Refit interfaces
// NB: This falls down in the tests unless we add an explicit "using Refit;",
// but we can rely on this being there in any other file
if (nodes.OfType<UsingDirectiveSyntax>().All(u => u.Name.ToFullString() != "Refit"))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doing this check here instead of in HasRefitHttpMethodAttribute means that the method could return true for something that's doesn't actually have a Refit attribute (i.e. one with a different "Get" or "Post" attribute). I'm pretty sure it doesn't matter as long as we have the check here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could roll down the "using" lists to make sure they reference Refit

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what you mean? I think that's what I'm doing here - my point was just that I'm doing it here and not in the method that looks for Refit attributes.

I could take the rolled up list all usings here and pass it in as a parameter, then it's not going to be traversing the whole tree each time the method is called. I'm conscious of the fact that this runs as part of the build so we need to make it as fast as possible. Maybe it's still micro-optimising though?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm dumb, I just read the comment and not the code, disregard :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👌

@bennor bennor changed the title Stub all the things Stub anything that looks like a Refit interface Oct 25, 2014
@bennor
Copy link
Contributor Author

bennor commented Oct 25, 2014

One more thing I've noticed: because we're looking for interfaces where any method has a Refit attribute and not all methods have Refit attributes, we could be stubbing methods cannot possibly work. Do you think it's worth:

  • changing it to require all methods to have a Refit attribute; or
  • stubbing methods without Refit attributes differently (to throw NotImplementedException or something); or
  • can we stub them as partials somehow - allowing people to define their own method bodies? (This could remove the need to define custom methods as extensions.)

Throw a `NotImplementedException` when someone calls a method that we
won't have an implementation for.
@bennor
Copy link
Contributor Author

bennor commented Oct 31, 2014

@paulcbetts I think this is ready to go.

  • I made the call to not allow anything but string literals for the path parameter for attributes. (I will happily do the extra work to allow constants the first time someone complains :trollface:)
  • In the interest of getting this out the door I will pull what to do for methods that have no attributes out to another issue -- I have a couple of ideas and I'm not sure which would be best. For now it just throws an exception that tells the user what they've done wrong.
  • I haven't updated the readme because the only part of this that I think could be documented is that constants are not supported in the HTTP method attributes, and I'm not sure we should draw attention to it - someone will raise it as a bug if they have a real world need to use one.

@anaisbetts anaisbetts merged commit 65e8a23 into reactiveui:master Nov 3, 2014
@anaisbetts
Copy link
Member

Looks great! Thanks @bennor!

@bennor bennor deleted the stub-all-the-things branch November 3, 2014 06:35
@promontis
Copy link

@paulcbetts could you perhaps push this to nuget?

@screamish
Copy link

Awesome! 👍

@lock lock bot locked and limited conversation to collaborators Jun 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stubs aren't generated unless service is instantiated in the same project
4 participants