Add filtered views #82
Conversation
| I'll review this once we have resolved #78. Sorry for the long wait. |
yt-ms force-pushed the issue-81-filtered-views branch from 1e7548c to ff4990b Compare 
Codecov Report
@@ Coverage Diff @@ ## devel #82 +/- ## ========================================== + Coverage 93.76% 94.00% +0.24% ========================================== Files 74 76 +2 Lines 2327 2404 +77 Branches 241 248 +7 ========================================== + Hits 2182 2260 +78 + Misses 119 116 -3 - Partials 26 28 +2
Continue to review full report at Codecov.
|
yt-ms force-pushed the issue-81-filtered-views branch from ff4990b to 4a1463f Compare 
yt-ms force-pushed the issue-81-filtered-views branch from 4a1463f to 5d4ffda Compare 
There was a problem hiding this comment.
Neat, I like that you created the abstract view class and it's nice to see how it's all coming together. I have some suggestions for improvements. The only major thing would be adding a dictionary attribute to the ViewSet class as mentioned in one of the comments. Not sure it's worth it right now.
| return { | ||
| "key": view_io.key, | ||
| "description": view_io.description, | ||
| "title": view_io.title, | ||
| } |
There was a problem hiding this comment.
I'm still not very happy with how we construct these arguments but that's neither here nor there.
| | ||
| base_view_key: str = Field(alias="baseViewKey") | ||
| mode: FilterMode | ||
| tags: List[str] |
There was a problem hiding this comment.
Take a look at ModelItemIO. We introduced two methods there to convert the tags to and from string which is what the structurizr API expects.
There was a problem hiding this comment.
Actually, the API is inconsistent, and for FilteredView the tags are serialised in JSON as an array. I've added the validator anyway for safety.
There was a problem hiding this comment.
I will try this against an actual Structurizr instance to make sure the Swagger is correct though...
There was a problem hiding this comment.
OK, I built a filtered view in Structurizr and exported the JSON, and it does indeed serialise the tags as an array rather than comma-separated.
There was a problem hiding this comment.
Huh, thanks for digging into that. @simonbrowndotje is that an oversight or fully intentional?
There was a problem hiding this comment.
It was intentional at one point in time, but not any more ... when I first started writing the Java client library (5+ years ago), I had some issues with the order of array elements being preserved. The order for tags defining a filtered view isn't important, but the ordering of element/relationship tags is. So the former is an array, whereas the latter is a comma-separated list.
| ) | ||
| return next((view for view in all_views if view.key == key), None) | ||
| | ||
| def __getitem__(self, key: str) -> AbstractView: |
There was a problem hiding this comment.
This method points a bit towards creating a 'private' attribute which is a dict of all the views. Then fetching a particular view by key is very easy. It would mean managing that dictionary, though.
There was a problem hiding this comment.
OK, so I've had a crack at this - actually replacing the various sets with a single dictionary. I think this actually works quite well - see what you think.
Co-authored-by: Moritz E. Beber <midnighter@posteo.net>
Co-authored-by: Moritz E. Beber <midnighter@posteo.net>
| Looks all good to me then. Thank you for the quick work on the changes. |
4 participants
Add support for filtered views,
FilteredViews are slightly odd in that they don't have their own layout (they just take from their base view). In the Java implementation, they actually don't inherit fromViewbecause of this even though there are some common fields - in Python, I've introduced a newAbstractViewsuperclass ofViewwhich is also common withFilteredView(I felt this was less disruptive than strippingViewdown and introducing a new type betweenViewand all its current subtypes..Also resolved a couple of TODOs in
ViewSet.This PR follows on from #78 so only commits from 6ef8af3 onwards are really part of this PR. I'll rebase once #78 is merged.
THIS SOFTWARE IS CONTRIBUTED SUBJECT TO THE TERMS OF THE Apache License v.2.0. YOU MAY OBTAIN A COPY OF THE LICENSE AT https://www.apache.org/licenses/LICENSE-2.0.
THIS SOFTWARE IS LICENSED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE AND ANY WARRANTY OF NON-INFRINGEMENT, ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. THIS SOFTWARE MAY BE REDISTRIBUTED TO OTHERS ONLY BY EFFECTIVELY USING THIS OR ANOTHER EQUIVALENT DISCLAIMER IN ADDITION TO ANY OTHER REQUIRED LICENSE TERMS.