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(apib): add format link #534

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

Conversation

marcofriso
Copy link
Contributor

@marcofriso marcofriso commented Aug 7, 2020

Closes APIARY-6103

@marcofriso marcofriso requested a review from kylef August 7, 2020 13:28
refractElement.element = result.element;
refractElement.meta = formatLink;
refractElement.content = result.content;
return refractElement;
Copy link
Member

Choose a reason for hiding this comment

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

One caveat here is that if the underlying parser added any other meta or attributes they would go missing. It might be a bit simpler to construct a minim elements from the given result. In options there is namespace which can be used as a minim namespace. You can get hold of a ParseResult element instance in the same way that the other parsers work. Returning a ParseResult here does not change the behaviour, the API Elements Core logic detects if the returned value is a Minim element, in the cases it is not, it will convert it to be one (thus doing what you'd be doing here).

const { namespace } = options;
const parseResult = namespace.fromRefract(source);
.. new namespace.elements.Link(...);
return parseResult;

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kylef in options there is not namespace, I don't know how to access it here. Seems to me that API Elements are managed internally by the drafter function, so by Protagonist.

Copy link
Member

@kylef kylef Aug 13, 2020

Choose a reason for hiding this comment

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

The options passed into the function:


function parse({
  source, generateSourceMap, generateMessageBody, generateMessageBodySchema,
  requireBlueprintName,
}) {

can be adjusted to add namespace. It doesn't need to be added to the options passed to drafter.

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

@marcofriso marcofriso requested a review from kylef August 17, 2020 10:17
if (!isAnnotation(parseResult.content[0])) {
const link = new Link();

link.title = 'Apiary Blueprint';
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
link.title = 'Apiary Blueprint';
link.title = 'API Blueprint';

This is the API Blueprint adapter

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

const isAnnotation = element => element.element === 'annotation';
const { Link } = namespace.elements;

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.

Can you please explain the intent of this check? I am not sure it is needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In case of error it does not add the format link, do you want to remove it?

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 in all cases we should include the link element, some editors such as Apiary show the source format on annotations which the link element's title will be useful for:

Screenshot 2020-08-17 at 17 03 48

The use-case may be finding the underlying format which caused the error. The "Unknown" here would become "OpenAPI" 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.

removed condition e310ea3

@marcofriso marcofriso requested a review from kylef August 17, 2020 15:12
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