davidmoten / openapi-to-plantuml

Converts OpenAPI 3.0 definitions to Plant UML text for visualisation of your API.
Apache License 2.0
95 stars 18 forks source link

Feature/add properties v3 #171

Open DimitriBoursier opened 1 year ago

davidmoten commented 1 year ago

Let me know when you want a review. I'll need a detailed description of your aims though. Oh and you need to pass CI too (see below).

DimitriBoursier commented 1 year ago

Release note

DimitriBoursier commented 1 year ago

Example image

davidmoten commented 1 year ago

Looks interesting. Have you checked the appearance of the other diagrams in demos? What's good for you might be a whole lot of mess for others. Probably boils down to configuration options which you have a TODO for. Default will probably be to leave that stuff out.

DimitriBoursier commented 1 year ago

Before bookstore After bookstore

DimitriBoursier commented 1 year ago

An option should be added to display the notes (yellow tag) (see TODO)

If we compare with before, links are lost I'll study

DimitriBoursier commented 1 year ago

Before image After image

DimitriBoursier commented 1 year ago

I think I'm done. Would you be interested in my contribution? I remain at your disposal for any questions.

davidmoten commented 1 year ago

Hi Dimitri Thanks for getting involved. My first comment is that there are too many enhancements mixed up in this PR. For the sanity of the reviewers you need to keep changes separate so that they can be discussed and worked on independently of other changes. This is something you pick up as you get into open-source contributions.

This is your summary:

Each of those contributions should be a separate PR but the best idea is to discuss what you want to do first so you don't put too much effort in to something that doesn't get accepted.

The output of this project is a UML Diagram that communicates the essence of the API, not the details. I'm not happy with the addition of string maxLength and notes because it seems like needless clutter. For example with the notes, in general the times that a description adds more than just the existing name of the field is when the description is long and detailed. In that specific case I also don't want that appearing as extra clutter. Here's a quick example of a long description (my apis have lots of long descriptions) that I definitely would not want in a diagram!

name: continuationToken
description: |
            Describes to the server the starting point of 
            the next page of results and is obtained from 
            the current page. May contain an offset if desired
            but is at the discretion of implementer. Note that
            it is possible that a call specifying a continuation
            token may return en empty list (but an empty list return 
            should not have a continuation token on it so at 
            that point paging would stop).

The {O} vs {R} usage is a style thing. Where have you seen {R} usage? Got links? I find {O} usage less noisy than {R} usage in general and that is apparent in the bookstore diagram above.

Add field in Class in addition link (like Diagram Class)

I need more info on this one.

There is a case on error when "array", so the type return object[]

I need more info on this effects of this one

So I'm not open to adding Note, size, and {R} instead of {O}. I don't see them as adding value but you can try and convince me!

The other two issues need more explanation and should be opened as separate PRs please.

I see your PR does not pass CI. This mean that running mvn clean install on the project is failing. Make sure you do that check before requesting review.

Another tip for PR contributions is to do all your work in your fork of the repo and only open a PR on the original repo when your PR is ready for review. That way I won't be drawn by notifications to inspect your changes only to see they are not ready for review.