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.

Sign up for GitHub

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

Jump to bottom

Migrate Addresses #653

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

Migrate Addresses #653

wants to merge 11 commits into from

Conversation

RobBeck1
Copy link
Contributor

Summary

Follow style guide

https://mews.atlassian.net/browse/CON-2195 Migrate Addresses

Check during review

  • The changelog and potentially a deprecation entries are in place.
    • The changelog timestamp is date of merge to develop.
  • JSON example extended.
    • New properties are added to the correct place in the JSON.
  • New properties in the table are added to the correct place.
  • Correct formatting:
    • Spacing is consistent between titles, sections, tables, ...
    • Correct JSON format - indentation.
  • DateTime properties should always be defined in ISO format.

@RobBeck1 RobBeck1 added Generator Pull requests related to reference documentation generator con.connectivity labels Sep 16, 2024
@RobBeck1 RobBeck1 self-assigned this Sep 16, 2024
@RobBeck1 RobBeck1 requested review from a team as code owners September 16, 2024 14:32
VBuben
VBuben reviewed Sep 17, 2024
View reviewed changes
operations/addresses.md Outdated Show resolved Hide resolved
operations/addresses.md Outdated Show resolved Hide resolved
operations/addresses.md Outdated Show resolved Hide resolved
operations/addresses.md Outdated Show resolved Hide resolved
operations/addresses.md Outdated
}
```

| Property | Type | Contract | Description |
| :-- | :-- | :-- | :-- |
| `Addresses` | array of [Account address](#account-address) | required | Created addresses. |
| `Addresses` | array of [Account address](addresses.md#account-address) | optional | The collection of Account addresses, containing address and account information. |
| `Cursor` | string | optional | Unique identifier of the last and hence oldest address item returned. This can be used in [Limitation](../guidelines/pagination.md#limitation) in a subsequent request to fetch the next batch of older Account address. |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's fix this, addresses/add should not have Cursor, because you don't iterate trough records, you are creating them 🚀.

operations/addresses.md Show resolved Hide resolved
operations/addresses.md Outdated
| `PostalCode` | [String update value](_objects.md#string-update-value) | optional, max length 255 characters | Postal code. |
| `CountryCode` | [String update value](_objects.md#string-update-value) | optional, max length 8 characters | ISO 3166-1 code of the Country. |
| `CountrySubdivisionCode` | [String update value](_objects.md#string-update-value) | optional, max length 8 characters | ISO 3166-2 code of the administrative division, e.g. DE-BW. |
| ~~`AccountId`~~ | ~~string~~ | ~~required~~ | **Deprecated!** The value is ignored.|
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why this value was not deleted, since the usage of it was removed 🤯
I think we can just remove it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, or hide it with InternalImplementation for starters.

operations/addresses.md Outdated Show resolved Hide resolved
jnv
jnv reviewed Sep 17, 2024
View reviewed changes
operations/addresses.md Outdated Show resolved Hide resolved
operations/addresses.md Outdated Show resolved Hide resolved
operations/addresses.md Outdated Show resolved Hide resolved
operations/addresses.md Outdated
| `PostalCode` | [String update value](_objects.md#string-update-value) | optional, max length 255 characters | Postal code. |
| `CountryCode` | [String update value](_objects.md#string-update-value) | optional, max length 8 characters | ISO 3166-1 code of the Country. |
| `CountrySubdivisionCode` | [String update value](_objects.md#string-update-value) | optional, max length 8 characters | ISO 3166-2 code of the administrative division, e.g. DE-BW. |
| ~~`AccountId`~~ | ~~string~~ | ~~required~~ | **Deprecated!** The value is ignored.|
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, or hide it with InternalImplementation for starters.

@RobBeck1
Implementing PR change requests (remove max string limit on AddressPa…
153767b 
…rameters CountryCode and CountrySubDivision, make Addressresult.Addresses required)
MikeAdamsMews
MikeAdamsMews approved these changes Sep 17, 2024
View reviewed changes
operations/addresses.md Outdated Show resolved Hide resolved
jonas-paul
jonas-paul reviewed Sep 18, 2024
View reviewed changes
_generator/types.yaml Outdated
file: _objects.md
file: vouchers.md
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did this change?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bug in generator, unit behaves strangely since it's a named type but doesn't have any properties, so for some reason the generator always overrides it. 🤔

jnv
jnv reviewed Sep 20, 2024
View reviewed changes
operations/addresses.md Outdated Show resolved Hide resolved
@MikeAdamsMews
Copy link
Contributor

Hey @RobBeck1 , this one's been sitting here awhile but I think there's enough info to resolve all the comments?

@RobBeck1
Copy link
Contributor Author

@MikeAdamsMews - oops, I didn't realise this was still here! I'll try and get it updated today. 👍

@RobBeck1
Copy link
Contributor Author

Hello @MikeAdamsMews - I re-generated the documentation from our existing code, and pushed the new addresses.md file, but it appears that there are other comments from after the commit of the associated back-end code. I will therefore create a new PR for the backend to make the additional changes in before regenerating the documentation again. Thanks.

@RobBeck1
Copy link
Contributor Author

@MikeAdamsMews - should all be done now :)

MikeAdamsMews
MikeAdamsMews approved these changes Nov 18, 2024
View reviewed changes
Copy link
Contributor

@MikeAdamsMews MikeAdamsMews left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks @RobBeck1

@MikeAdamsMews MikeAdamsMews requested a review from VBuben November 19, 2024 13:29
jnv
jnv reviewed Nov 19, 2024
View reviewed changes
_generator/types.yaml Outdated Show resolved Hide resolved
@jnv
Copy link
Member

jnv commented Nov 19, 2024

Consider adding a line into a changelog, something like:

Aligned Addresses API operations with OpenAPI Specification, adding previously undocumented properties and fixing examples. Documentation-only.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonas-paul jonas-paul jonas-paul left review comments

@MikeAdamsMews MikeAdamsMews MikeAdamsMews approved these changes

@jnv jnv jnv approved these changes

@VBuben VBuben Awaiting requested review from VBuben

Assignees

@RobBeck1 RobBeck1

Labels
con.connectivity Generator Pull requests related to reference documentation generator
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@RobBeck1 @MikeAdamsMews @jnv @jonas-paul @VBuben