Allow for passing in Definition when loading OpenAPI definitions#356
Allow for passing in Definition when loading OpenAPI definitions#356
Conversation
| # Verify the plan was registered with the definition key | ||
| expect(plan).to be_a(OpenapiFirst::Test::Coverage::Plan) | ||
| expect(plan.definition_key).to eq(expected_key) | ||
| expect(plan.api_identifier).to eq(expected_key) |
There was a problem hiding this comment.
I've created a definition_key and api_identifier on the plan. However, it seems like they'll always be the same. Thoughts? Maybe always use api_identifier?
There was a problem hiding this comment.
Agreed. api_identifier is the better option here.
| "Please add these fields to your OpenAPI document." | ||
| end | ||
|
|
||
| "#{title} @ #{version}" |
There was a problem hiding this comment.
Thoughts? You mentioned using a hash of the definition. Would you prefer that over this?
There was a problem hiding this comment.
Until now openapi_first did not have an opinion about the contents of the OAD…, but I think its' fine the way you did it, because it will lead to a human readable key and the code is easy to follow.
| # @param filepath_or_definition [String, Definition] The path to the file or a Definition object | ||
| # @return [Definition] | ||
| def self.load(filepath, only: nil, &) | ||
| def self.load(filepath_or_definition, only: nil, &) |
There was a problem hiding this comment.
Why do you want .load to be able to accept a Definition instance? In order to avoid something like if path_or_definition.is_a?(Definition) in test.rb?
There was a problem hiding this comment.
I was treating .load as a single place to handle this. A definition or file path goes in at the edges and then you always get a definition. However, if you prefer to not do that, I'm happy to change it! I leave to you.
|
|
||
| def self.for(oad, skip_response: nil) | ||
| plan = new(filepath: oad.filepath) | ||
| plan = new(definition_key: oad.key, filepath: oad.filepath) |
There was a problem hiding this comment.
I understand why you changed this. This makes sense to me.
| after(:each) do | ||
| described_class.uninstall | ||
| described_class.reset | ||
| OpenapiFirst::Test.definitions.clear |
There was a problem hiding this comment.
OpenapiFirst::Test.definitions.clear is already called globally after each test. See spec_helper.rb. This this should not be necessary here.
| def initialize(definition_key:, filepath: nil) | ||
| @routes = [] | ||
| @index = {} | ||
| @definition_key = definition_key |
There was a problem hiding this comment.
Let's set @api_identifier right here and remove the definition_key reader.
|
|
||
| ```ruby | ||
| existing_definition = OpenapiFirst.parse(...) # Returns an Openapi::Definition object | ||
| definition = OpenapiFirst.load(existing_definition) # Returns the same Definition object |
There was a problem hiding this comment.
I think describing this special behavior of OpenapiFirst.load (being able to accept a Definition instance) is too much for the Readme. Let's remove that okay?
But I like mentioning OpenapiFirst.parse somewhere in the Readme, because some people might need that, right?
| end | ||
|
|
||
| it 'uses filepath as key for Definition objects with filepath' do | ||
| # Start coverage tracking |
There was a problem hiding this comment.
I appreciate you taking the time to get this deep into the internal APIs.
| # Start coverage tracking | ||
| OpenapiFirst::Test::Coverage.install | ||
|
|
There was a problem hiding this comment.
No need to call .install for this test to pass
| # Start coverage tracking | |
| OpenapiFirst::Test::Coverage.install |
| # Start coverage tracking | ||
| OpenapiFirst::Test::Coverage.install | ||
|
|
There was a problem hiding this comment.
| # Start coverage tracking | |
| OpenapiFirst::Test::Coverage.install |
|
@ahx I've tried to address your feedback. Let me know if I missed something! |
|
@smizell Do you need a new version released with this soon? I can take of that, but if you have some time I would like to look at two other issues before releasing a new version. |
|
No rush on my part! Thanks for merging this in! |
2 participants
This implements the functionality discussed in #353. The approach modifies the
Openapi.loadmethod to take either a Definition or a String. It then modifies the Test Coverage functionality to allow registering a Definition object. To make this work, this PR adds aDefinition#keymethod that builds a key from theinfo.titleandinfo.version. If those aren't present, then it raises an error.