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

feat(oas3): add format link #533

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Conversation

marcofriso
Copy link
Contributor

Closes APIARY-6106

@marcofriso marcofriso changed the title Marco/oas3 format link feat(oas3): add format link Aug 6, 2020
@marcofriso marcofriso requested a review from kylef August 6, 2020 14:31
@marcofriso
Copy link
Contributor Author

Why are not there package-lock.json files? Should I add them to .gitignore?

@kylef
Copy link
Member

kylef commented Aug 6, 2020

Yarn is used, not npm. There is yarn lockfile in root of repo, there is no need to run npm install here, you can delete the files and use yarn: (https://github.com/apiaryio/api-elements.js#using-lerna).

Copy link
Member

@kylef kylef left a comment

Choose a reason for hiding this comment

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

I've commented on some cases where the changes in lib/parser.js can return an undesired output given certain inputs. I'd suggest using the document variable to retieve the version. document will contain the parsed YAML/JSON document as Minim elements (wrapped in parse result). You could use something along the lines of document.content.find(isObject) to get the "object" element which represent the tree. The getValue('openapi') on an object element can get you the value of the key (which might be a string element, you would want to confirm that is true).

packages/openapi3-parser/CHANGELOG.md Outdated Show resolved Hide resolved
packages/openapi3-parser/lib/parser.js Outdated Show resolved Hide resolved
@marcofriso marcofriso requested a review from kylef August 13, 2020 14:45
packages/openapi3-parser/lib/parser.js Outdated Show resolved Hide resolved
R.pipe(
parseDocument,
deduplicateUnsupportedAnnotations(context.namespace),
context.options.generateSourceMap ? filterColumnLine : filterSourceMaps
),
document
);

if (!isAnnotation(parseResult.content[0])) {
Copy link
Member

Choose a reason for hiding this comment

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

I think this check can be removed like in the API Blueprint version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

R.pipe(
parseDocument,
deduplicateUnsupportedAnnotations(context.namespace),
context.options.generateSourceMap ? filterColumnLine : filterSourceMaps
),
document
);

if (!isAnnotation(parseResult.content[0])) {
const formatVersion = R.pipe(
Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest adding a check here for the value of the openapi member element being a string element, and if any of these checks fail I think we should default to the latest OpenAPI 3 version we know about which is 3.0.3. I think there may be cases where we could get here with an invalid document which cannot be parsed correctly.

Making a test similar to it('add the format link', () => { but instead the source is invalid document should verify this, if you set source to be an unexpected value such as const source = '{}' or const source = '[]' etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed here f52cd03

Copy link
Contributor Author

Choose a reason for hiding this comment

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

check if the value of the OpenAPi member element is a string element 88bcb56

@marcofriso marcofriso requested a review from kylef August 18, 2020 13:48
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.

None yet

2 participants