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

Changes in schema & guide #1080

Merged
merged 1 commit into from
Oct 13, 2022
Merged

Changes in schema & guide #1080

merged 1 commit into from
Oct 13, 2022

Conversation

Imod7
Copy link
Contributor

@Imod7 Imod7 commented Oct 9, 2022

Description

The goal of this PR was mainly to close the issue #1076 but added some other minor changes too.

Changes

In the OpenAPI specs :

  • Moved the /accounts/{accountId}/convert path under /accounts/{accountId}/balance-info so it is in alphabetical order.
  • Added the AccountConvert schema (which was missing as mentioned in the issue 1076).
  • Added items of type: string under the PalletStorageItem schema because otherwise it was showing the following error in the Swagger Editor
    Semantic error at components.schemas.PalletStorageItem.properties.keys
    Schemas with 'type: array', require a sibling 'items: ' field
    

In the CONTRIBUTING guide :

  • Added the step of verifying with the swagger editor before rebuilding the docs. It is already mentioned in the docs README but maybe it is useful to have it also in the contributing page assuming that potential contributors will mainly check the contributing guide.

Todos

[x] Verify changes with the swagger editor
[x] Rebuild the docs

- Moved the `AccountConvert` path so it is in alphabetical order.
- Added the `AccountConvert` schema.
- Small correction in `PalletStorageItem` schema.
- Added swagger verification step in the CONTRIBUTING guide.
@Imod7 Imod7 requested a review from a team as a code owner October 9, 2022 07:52
@jsdw
Copy link
Collaborator

jsdw commented Oct 10, 2022

Moved the /accounts/{accountId}/convert path under /accounts/{accountId}/balance-info so it is in alphabetical order.

Much like we do with code formatting, we could run something like https://www.npmjs.com/package/openapi-format in CI to complain if it's not formatted (and possibly catch a few issues; not sure). Committers would then be expected to run something likenpm run openapi-format to fix it if it's an issue.

Just a thought though :)

@Imod7
Copy link
Contributor Author

Imod7 commented Oct 10, 2022

Much like we do with code formatting, we could run something like https://www.npmjs.com/package/openapi-format in CI to complain if it's not formatted (and possibly catch a few issues; not sure). Committers would then be expected to run something likenpm run openapi-format to fix it if it's an issue.

Just a thought though :)

I think that is a great idea! 💯 I will check what commands/jobs we need to add in the .gitlab-ci.yml file to have this check/fix in CI.

Copy link
Collaborator

@jsdw jsdw left a comment

Choose a reason for hiding this comment

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

That aside, looks good to me! I'd suggest that the above changes be a new PR anyway; no need to stop this fix from merging :)

@jsdw jsdw requested a review from marshacb October 10, 2022 16:06
@Imod7 Imod7 merged commit 3fa9689 into master Oct 13, 2022
@Imod7 Imod7 deleted the domi-fix-convert-schema branch October 13, 2022 10:27
This pull request was closed.
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.

4 participants