Skip to content

Keep order of params when generating full spec #204

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
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

d-mo
Copy link

@d-mo d-mo commented Sep 27, 2020

What does this PR do?

This PR utilizes yamlloader.ordereddict when loading the input yaml files. This way the order of the attributes does not change when generating the full specification.

Review checklist (to be filled by reviewers)

  • Feature or bug fix MUST have appropriate tests (unit, integration, etc...)
  • PR title must be written as a CHANGELOG entry (see why)
  • Files changes must correspond to the primary purpose of the PR as described in the title (small unrelated changes should have their own PR)
  • PR must have one changelog/ label attached. If applicable it should have the backward-incompatible label attached.
  • PR should not have do-not-merge/ label attached.
  • If Applicable, issue must have kind/ and severity/ labels attached at least.

@d-mo d-mo requested a review from a team as a code owner September 27, 2020 13:41
Copy link
Contributor

@jirikuncar jirikuncar left a comment

Choose a reason for hiding this comment

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

@d-mo can you please use black to format edited files and add a test case?

@d-mo
Copy link
Author

d-mo commented Oct 20, 2020

@jirikuncar Done

@github-actions
Copy link

This issue has been automatically marked as stale because it has not had activity in the last 30 days.
Note that the issue will not be automatically closed, but this notification will remind us to investigate why there's been inactivity.

@github-actions github-actions bot added the stale Stale - Bot reminder label Nov 20, 2020
@d-mo d-mo requested a review from jirikuncar December 11, 2020 13:42
jirikuncar
jirikuncar previously approved these changes Sep 6, 2021
@jirikuncar jirikuncar added stale/exempt Exempt from being marked as stale and removed stale Stale - Bot reminder labels Sep 6, 2021
@d-mo d-mo force-pushed the maintain-param-order branch from 123173f to a431a05 Compare September 10, 2021 11:39
@d-mo
Copy link
Author

d-mo commented Sep 13, 2021

Hi @jirikuncar

I rebased the PR and @ckarageorgkaneen extended it to include the dicts under the components section.

Are you interested in merging it? Is there anything else missing?

d-mo and others added 2 commits September 15, 2021 22:10
@d-mo
Copy link
Author

d-mo commented Sep 16, 2021

@jirikuncar Thank you for the suggestions! I applied both.

@jirikuncar
Copy link
Contributor

@d-mo I have done some testing and it is way slower when merging our large OpenAPI specifications.

# master
$ time apigentools merge

real    0m1.612s
user    0m1.536s
sys     0m0.067s

# pr/204
$ time apigentools merge

real    0m10.816s
user    0m10.577s
sys     0m0.107s

Can we find a way that we could still use the CSafeDumper and CSafeLoader together with the OrderedDict?

@d-mo
Copy link
Author

d-mo commented Sep 25, 2021

@jirikuncar Can you test again? I was able to use CSafeLoader & CSafeDumper after switching to yamlloader.

@jirikuncar
Copy link
Contributor

@d-mo it seems fine now. I don't see any major performance issue on large files.

jirikuncar
jirikuncar previously approved these changes Sep 27, 2021
@d-mo
Copy link
Author

d-mo commented May 10, 2022

Hi @jirikuncar.

I fixed the conflicts that came up. Is there any interest in merging this PR at some point?

We'd love to use the official apigentools instead of maintaining our own fork. I think the changes might be useful to others as well.

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale/exempt Exempt from being marked as stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants