Use x-navtitle and x-slug extensions for tags. Close #50
#52
Conversation
Use `x-navtitle` as a title for tag in ToC and page header. Use `x-slug` as a directory name for tag and it's endpoints.
alexeyten
changed the title
x-navtitle and x-slug extensions for tags (fixes #50)x-navtitle and x-slug extensions for tags (Close #50)
alexeyten
changed the title
x-navtitle and x-slug extensions for tags (Close #50)x-navtitle and x-slug extensions for tags. Close #50
There was a problem hiding this comment.
Overall, I think this is a solid way to support more vendor extensions in a way that makes sense.
However, I'm a bit concerned about these changes interacting with previously defined piece of logic used to derive tag titles from endpoint -> x-navtitle, which was an array type in contrast to this PR's basic string usage of x-navtitle.
src/includer/parsers.ts
Outdated
| @@ -110,6 +110,15 @@ function tagsFromSpec(spec: OpenAPISpec): Map<string, Tag> { | |||
| } | |||
| } | |||
|
|
|||
| parsed.forEach((tag: Tag) => { | |||
| if (tag['x-navtitle']) { | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
src/includer/parsers.ts
Outdated
| @@ -110,6 +110,15 @@ function tagsFromSpec(spec: OpenAPISpec): Map<string, Tag> { | |||
| } | |||
| } | |||
|
|
|||
| parsed.forEach((tag: Tag) => { | |||
| if (tag['x-navtitle']) { | |||
| tag.name = tag['x-navtitle']; | |||
There was a problem hiding this comment.
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.
src/includer/parsers.ts
Outdated
| tag.name = tag['x-navtitle']; | ||
| } | ||
| if (tag['x-slug']) { | ||
| tag.id = tag['x-slug']; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
src/__tests__/tags.test.ts
Outdated
| describe('tags rendering', () => { | ||
| it('renders tag and toc', async () => { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Also, I think it's not a bad idea to split this test case, since this looks to me like three independent test cases.
paths:
/test:
post:
......
tags:
- tag1
- tag2
x-navtitle:
- Title for tag1
- Title for tag2
/other:
post:
......
tags:
- tag1
- tag3
x-navtitle:
- New title for tag1 # will override title from endpoint above
- Title for tag3
tags:
- name: tag1
description: Description for tag1
- name: tag2
description: Description for tag2
- name: tag3
x-navtitle: New title for tag3 # will override any title from endpointsWill result in tags:
- id: tag1
name: New title for tag1 # from paths./other.post
description: Description for tag1
- id: tag2
name: Title for tag2 # from paths./test.post
description: Description for tag2
- id: tag3
name: New title for tag3 # from tags[name=tag3] |
True. I don't think I was clear enough in my original comment — my issue with this interaction is that after merging the proposed changes, the spec will end up with two distinct ways of using the Also, I think your example really highlights reasons why I think having As I see it, this usage is a) non-descriptive and confusing, and b) introduces more potential pitfalls. Also, it just makes sense to define diplodoc section names in Even if we don't end up deprecating the old behavior, I would argue in favor of choosing a different property identifier for the new behavior. Anyway, I would like to hear your thoughts on this. I would like to hear @3y3 @v8tenko 's opinion on this as well. |
3y3
force-pushed
the
master
branch
2 times, most recently
from
545f9c4
to
e5eacec
Compare
Use
x-navtitleas a title for tag in ToC and page header.Use
x-slugas a directory name for tag and it's endpoints.Fixes #50