Update resource model format. #51
Merged
Conversation
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
danielgtaylor
added
the
enhancement
This updates to the latest resource model format, which includes the following
changes:
1. `belongsTo` references are now named `has`
2. `subResources` is replaced by two-way `has` references
3. Identifier and parameter definitions are refactored:
* `source` is now one of `name`, `path` or `value`
* `sourceType` is now `source`
* A new source of `input` is added for user-supplied input
Our interface remains the same, but the underlying JSON files have changed
and our factory code and documentation are somewhat simplified. The code
that handles identifiers/parameters is a bit more complex due to having
to check several keys in the dictionary based on the source.
The model still exposes subresources and references as separate concepts.
Due to circular references, resource instances now lazy-load the subresource
class when needed.
Tests and documentation have been updated to reflect these changes.
danielgtaylor
force-pushed
the
resource-model-update
branch
from
3574b5b
to
6b6cf76
Compare
| for key, value in identifiers.items(): | ||
| pargs.append(getattr(self, xform_name(key))) | ||
| for identifier, value in build_identifiers(identifiers, self): | ||
| pargs.append(value) |
There was a problem hiding this comment.
Nevermind I was only looking at the diff. It is positional args
There was a problem hiding this comment.
Not a big deal, but I prefer the vars spelled out to avoid confusion, positional_args would work for me.
|
It looks good to me. 🚢 Most of my comments were minor: questions and comments on style and tests. |
|
Overall looks good. Just had a couple questions. |
|
Any ideas on the coverage drop? |
|
Code coverage drop is likely due to removed lines. All new lines are covered, but the few uncovered lines in e.g. |
danielgtaylor
added a commit
that referenced
this pull request
Feb 3, 2015
Update resource model format.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This updates to the latest resource model format, which includes the following
changes:
belongsToreferences are now namedhassubResourcesis replaced by two-wayhasreferencessourceis now one ofname,pathorvaluesourceTypeis nowsourceinputis added for user-supplied inputOur interface remains the same, but the underlying JSON files have changed
and our factory code and documentation are somewhat simplified. The code
that handles identifiers/parameters is a bit more complex due to having
to check several keys in the dictionary based on the source.
The model still exposes subresources and references as separate concepts.
Due to circular references, resource instances now lazy-load the subresource
class when needed.
Tests and documentation have been updated to reflect these changes.
Note: Before merging, this requires a JMESPath update to support
@as a root selector, which is the format used in the models now.cc: @jamesls @kyleknap