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

Use x-navtitle and x-slug extensions for tags. Close #50 #52

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 53 additions & 0 deletions src/__snapshots__/tags.test.ts.snap
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`tags rendering renders tag and toc: basic 1`] = `
"# basic

basic tag

## Endpoints

- [tags](tags.md)

<!-- markdownlint-disable-file -->"
`;

exports[`tags rendering renders tag and toc: toc 1`] = `
"name: openapi
items:
- name: Overview
href: index.md
- name: basic
items:
- name: Overview
href: basic/index.md
- href: basic/tags.md
name: tags
- name: Alternate title
items:
- name: Overview
href: navtitle/index.md
- href: navtitle/tags.md
name: tags
- name: Normal title
items:
- name: Overview
href: normal/index.md
- href: normal/tags.md
name: tags
- href: test-parameters.md
name: parameters test
"
`;

exports[`tags rendering renders tag and toc: weird 1`] = `
"# Normal title

weird tag with normal slug

## Endpoints

- [tags](tags.md)

<!-- markdownlint-disable-file -->"
`;
19 changes: 19 additions & 0 deletions src/__tests__/__helpers__/run.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,16 @@ import {dump} from 'js-yaml';
import {virtualFS} from './virtualFS';
import nodeFS from 'fs';

declare module 'openapi-types' {
// eslint-disable-next-line @typescript-eslint/no-namespace
namespace OpenAPIV3 {
interface TagObject {
'x-navtitle'?: string;
'x-slug'?: string;
}
}
}

const baseDocument = {
openapi: '3.0.2',
info: {
Expand Down Expand Up @@ -37,6 +47,7 @@ export class DocumentBuilder {
private responses: [code: number, response: OpenAPIV3.ResponseObject][] = [];
private parameters: OpenAPIV3.ParameterObject[] = [];
private components: Record<string, OpenAPIV3.SchemaObject> = {};
private tags: Array<OpenAPIV3.TagObject> = [];
private requestBody?: OpenAPIV3.RequestBodyObject = undefined;

constructor(id: string) {
Expand Down Expand Up @@ -86,6 +97,12 @@ export class DocumentBuilder {
return this;
}

tag(schema: OpenAPIV3.TagObject) {
this.tags.push(schema);

return this;
}

build(): string {
if (!this.responses.length) {
throw new Error("Test case error: endpoint can't have no responses");
Expand All @@ -99,13 +116,15 @@ export class DocumentBuilder {
requestBody: this.requestBody,
operationId: this.id,
responses: Object.fromEntries(this.responses),
tags: this.tags.map((tag) => tag.name),
},
parameters: this.parameters,
},
},
components: {
schemas: this.components,
},
tags: this.tags,
};

return dump(spec);
Expand Down
51 changes: 51 additions & 0 deletions src/__tests__/tags.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
import {DocumentBuilder, run} from './__helpers__/run';

const name = 'tags';
describe('tags rendering', () => {
it('renders tag and toc', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think these test names poorly reflect on what's actually being tested. This PR is implementing a mechanism to override page headings and URLs when specific vendor extensions are used. I could suggest naming the test suite something along the lines of Identity of pages derived from spec tags with cases named like can be overriden using x-slug to define a custom URL. Anyway, I'm open to suggestions here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, I think it's not a bad idea to split this test case, since this looks to me like three independent test cases.

Copy link
Author

Choose a reason for hiding this comment

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

I've split test cases

const spec = new DocumentBuilder(name)
.response(200, {
description: 'Base 200 response',
schema: {
type: 'object',
properties: {
type: {
type: 'string',
},
foo: {
type: 'string',
},
},
},
})
.tag({
name: 'basic',
description: 'basic tag',
})
.tag({
name: 'navtitle',
'x-navtitle': 'Alternate title',
})
.tag({
name: 'weird',
description: 'weird tag with normal slug',
'x-slug': 'normal',
'x-navtitle': 'Normal title',
})
.build();

const fs = await run(spec);

const toc = fs.match('toc.yaml');

expect(toc).toMatchSnapshot('toc');

const tag = fs.match('basic/index.md');

expect(tag).toMatchSnapshot('basic');

const tag2 = fs.match('normal/index.md');

expect(tag2).toMatchSnapshot('weird');
});
});
8 changes: 4 additions & 4 deletions src/includer/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ async function generateToc(params: GenerateTocParams): Promise<void> {
items: [],
};

tags.forEach((tag, id) => {
tags.forEach((tag) => {
// eslint-disable-next-line no-shadow
const {name, endpoints: endpointsOfTag} = tag;

Expand All @@ -165,7 +165,7 @@ async function generateToc(params: GenerateTocParams): Promise<void> {
};

const custom = ArgvService.tag(tag.name);
const customId = custom?.alias || id;
const customId = custom?.alias || tag.id;

section.items = endpointsOfTag.map((endpoint) => handleEndpointRender(endpoint, customId));

Expand Down Expand Up @@ -273,11 +273,11 @@ async function generateContent(params: GenerateContentParams): Promise<void> {
});
}

spec.tags.forEach((tag, id) => {
spec.tags.forEach((tag) => {
const {endpoints} = tag;

const custom = ArgvService.tag(tag.name);
const customId = custom?.alias || id;
const customId = custom?.alias || tag.id;

endpoints.forEach((endpoint) => {
results.push(handleEndpointIncluder(endpoint, join(writePath, customId), sandbox));
Expand Down
2 changes: 2 additions & 0 deletions src/includer/models.ts
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,8 @@ export type Tag = {
name: string;
description?: string;
endpoints: Endpoints;
'x-navtitle'?: string;
'x-slug'?: string;
};

export type Endpoints = Endpoint[];
Expand Down
9 changes: 9 additions & 0 deletions src/includer/parsers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,15 @@ function tagsFromSpec(spec: OpenAPISpec): Map<string, Tag> {
}
}

parsed.forEach((tag: Tag) => {
if (tag['x-navtitle']) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume this is supposed to make prior x-navtitle resolution logic (defined in this file on L94) obsolete, would you say that is correct?

Copy link
Author

Choose a reason for hiding this comment

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

Yes. It overrides x-navtitle from endpoints. It was always looks very unnatural and error-prone to me. I mean, what if different endpoints uses different x-navtitle for the same tag?

But I'm trying to keep backward compatibility.

tag.name = tag['x-navtitle'];
Copy link
Contributor

Choose a reason for hiding this comment

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

I have my reservations about the fact that we have zero distinction between a Tag that originates from an OpenAPI spec and a Tag that gets actually used when generating the ToC. I consider having two distinct entity types a better solution, where the includer-specific entity would bear no references to spec-specific vendor extensions such as x-navtitle or x-slug. However, I do recognize that this is out of scope for this PR, since this would require a significant rewrite of these parsers/transformers.

}
if (tag['x-slug']) {
tag.id = tag['x-slug'];
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we're now modifying tag.id based on x-slug's value, I would argue this would actually break a (loose) contract on Map<string, Tag> we had prior to these changes, i.e. a Tag's name now may or may not correspond to the key under which it is stored in the map.

From my point of view, this fact makes using a map obsolete, and we should probably collect the tags in a list instead.

Copy link
Author

Choose a reason for hiding this comment

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

That's why I had to modify includer/index.ts.

But we could see this as Map<OpenAPI_tag_name, diplodoc_section>, we still need to collect all tags from endpoints in OpenAPI spec file.

}
});

return parsed;
}
const opid = (path: string, method: string, id?: string) => slugify(id ?? [path, method].join('-'));
Expand Down