-
Couldn't load subscription status.
- Fork 0
feat: performance optimization for changelog build type
#36
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
base: develop
Are you sure you want to change the base?
Conversation
| version: 0.1.0 | ||
| paths: | ||
| /api/v1/path1: | ||
| get: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nothing changed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
content type changed from application/xml to application/json
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so then rename change-method to change-content-type :)
| version: 0.1.0 | ||
| paths: | ||
| /api/v1/path1: | ||
| get: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nothing changed (before)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
content type changed from application/xml to application/json
…e-optimization # Conflicts: # package.json
| let operationDiffs: Diff[] = [] | ||
| if (operationChanged) { | ||
| operationDiffs = [...(methodData as WithAggregatedDiffs<GraphApiOperation>)[DIFFS_AGGREGATED_META_KEY]??[]] | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
format
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Formatted
| prevDocData = createCopyWithEmptyPathItems(currDocData) | ||
| } | ||
| if (prevDocData && !currDocData) { | ||
| currDocData = createCopyWithEmptyPathItems(prevDocData) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it possible to have collisions here? I mean, can prevDocData and currDocData be overridden in second if operators?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Collision- no. Added comments to clarify what cases are handled by each piece. This is sequential processing and result of first step (if there are prefix groups) should still be processed by second step (create empty counterpart document if necessary)
src/apitypes/rest/rest.types.ts
Outdated
| // externalDocs?: ExternalDocumentationObject; | ||
| // 'x-express-openapi-additional-middleware'?: (((request: any, response: any, next: any) => Promise<void>) | ((request: any, response: any, next: any) => void))[]; | ||
| // 'x-express-openapi-validation-strict'?: boolean; | ||
| // } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't we remove it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
|
|
||
| const groupSlug = convertToSlug(areOperationsFromCurrentVersion ? currentGroup : previousGroup) | ||
| function removeRedundantPartialPairs<T extends [object | undefined, object | undefined]>( | ||
| tuples: T[], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add unit tests for such not trivial utilities
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added unit tests for dedupeTuples and removeRedundantPartialPairs functions
| }) | ||
|
|
||
| test('comparison should have 1 breaking and 1 annotation changes', async () => { | ||
| test('Comparison should have 1 breaking and 1 annotation changes', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comparison of...what?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed the name of the test group to make it clear these are tests for changelog build type.
| title: test | ||
| version: 0.1.0 | ||
| servers: | ||
| - url: https://example1.com/api/v2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this in before.yaml?
| version: 0.1.0 | ||
| servers: | ||
| - url: https://example2.com | ||
| description: It is a description of server |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is it in after.yaml?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Switched before and after to avoid confusion.
| @@ -1 +1 @@ | |||
| # test | |||
| # test | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this file necessary for tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The file was changed in the PR due to linter errors.
This is an existing test called multifiles, so I wouldn't change it here.
…nd path item servers object when comparing prefix groups
… as close to source as possible
…o avoid confusion
build type:
changelogbuild type:
build