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

Feature/add properties v3 #171

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

Conversation

DimitriBoursier
Copy link

No description provided.

@davidmoten
Copy link
Owner

davidmoten commented Aug 16, 2023

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
Copy link
Author

Release note

  • Add Size for string (maxLength)
  • Add Note in Diagram (in Note there are Description, Example, Format, Extension)
  • Add field in Class in addition link (like Diagram Class)
  • Replace {O} (optional) by {R} (Require)
  • There is a case on error when "array", so the type return object[]

@DimitriBoursier
Copy link
Author

Example
image

@davidmoten
Copy link
Owner

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.

- Extend properties on other case
- Add example
@DimitriBoursier
Copy link
Author

Before
bookstore
After
bookstore

@DimitriBoursier
Copy link
Author

DimitriBoursier commented Aug 17, 2023

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
Copy link
Author

Before
image
After
image

@DimitriBoursier
Copy link
Author

DimitriBoursier commented Aug 18, 2023

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

@davidmoten
Copy link
Owner

davidmoten commented Aug 22, 2023

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:

  • Add Size for string (maxLength)
  • Add Note in Diagram (in Note there are Description, Example, Format, Extension)
  • Add field in Class in addition link (like Diagram Class)
  • Replace {O} (optional) by {R} (Require)
  • There is a case on error when "array", so the type return object[]

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.

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.

None yet

2 participants