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

Fix - Clear Yard Registry to use swagger_yard with Sinatra splited routes files #68

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

AntoineGirard
Copy link

@AntoineGirard AntoineGirard commented Jun 15, 2021

Allow to use it, with Sinatra routes splited in multiple files.

Sinatra doesn't have the concept of controller like Rails. If you want to split your routes in multiple files you need to do something like this.

Before split

MyApp < Sinatra::Application
  get "/user/{id}" do
    
  end
  
  post "/user" do
    
  end
  
  get "/article/{id}" do
    
  end

  
end

After split

user.rb

# @resource User
MyApp < Sinatra::Application
  # Returns user
  #
  # @path [GET] /user/{id}
  get "/user{id}" do
    
  end
  
  # Save and returns user
  #
  # @path [POST] /user
  post "/user" do
    
  end
end

article.rb

# @resource Article
MyApp < Sinatra::Application
  # Returns article
  #
  # @path [GET] /article/{id}
  get "/article/{id}" do
    
  end
  
  # Save and returns article
  #
  # @path [POST] /article
  post "/article" do
    
  end
end

In this case, only the first file documentation is returned by Swagger#to_h if your config.controller_path is set to routes/**/* because Yard save methods of all files in class MyApp associated to the first file analyzed path.

Example:

Specification#parse_controllers analyzed my previous file in this order : user -> article.

  • During user analysis, SwaggerYard#yard_objects_from_file save MyApp and methods get user and post user to the Yard Registry. And ::YARD::Registry.all(*types).select {|co| co.file == file_path } return class and methods.
  • During article analysis, SwaggerYard#yard_objects_from_file add methods get article and post article to the Yard Registry. Like MyApp is associated to user.rb path, ::YARD::Registry.all(*types).select {|co| co.file == file_path } returns empty array.

By clearing Yard Registry each time, this permit to return methods for each file and tags associated to these methods.

It shouldn't have side effects cause Yard::Registry is only use here.

I hope that my explanations are clear enough. If you have any comments or questions, please let me know.

@AntoineGirard AntoineGirard marked this pull request as ready for review June 15, 2021 13:54
@AntoineGirard AntoineGirard changed the title Fix - Clear Yard Registry Fix - Clear Yard Registry to use swagger_yard with Sinatra splited routes files Jun 15, 2021
@nicksieger
Copy link
Collaborator

Hey Antoine, thanks for this contribution! The only issue I'll point out is that back in v0.3.1 the caching functionality of the registry is specifically called out, and is useful in my opinion for larger codebases when using, for example, the swagger controller in swagger_yard-rails. I wonder if we could either make this clearing behavior configurable or find another way to solve your issue. I'll try to think of other ways of solving this too. It might help to have a test case to demonstrate the behavior also. Let me know if you want to try that and need any help with it.

@AntoineGirard AntoineGirard force-pushed the fix/clear-yard-registry branch 3 times, most recently from 1a080ab to 47d4bcb Compare November 4, 2021 10:04
@AntoineGirard
Copy link
Author

Hey @nicksieger
Thanks for your answer. I didn't see this side effect. For now, I think the best solution is to make this cache usage configurable. I proposed an implementation. I think we could find another solution but I have no idea for now.

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.

2 participants