Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Feature/add properties v3 #171

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

Open
DimitriBoursier wants to merge 15 commits into davidmoten:master
base: master
Choose a base branch
Loading
from DimitriBoursier:feature/add-properties-v3

Conversation

Copy link

@DimitriBoursier DimitriBoursier commented Aug 14, 2023

No description provided.

Copy link
Owner

davidmoten commented Aug 16, 2023
edited
Loading

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).

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[]

Copy link
Author

Example
image

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.

Copy link
Author

Before
bookstore
After
bookstore

Copy link
Author

DimitriBoursier commented Aug 17, 2023
edited
Loading

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

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

davidmoten reacted with thumbs up emoji

Copy link
Author

Before
image
After
image

Copy link
Author

DimitriBoursier commented Aug 18, 2023
edited
Loading

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

Copy link
Owner

davidmoten commented Aug 22, 2023
edited
Loading

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

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /