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

Adds the graph schemaextension get cmd #14 #886

Closed
wants to merge 1 commit into from
Closed

Adds the graph schemaextension get cmd #14 #886

wants to merge 1 commit into from

Conversation

ypcode
Copy link
Contributor

@ypcode ypcode commented Mar 23, 2019

No description provided.

@ypcode ypcode changed the title Adds the graph schemaextension get cmd PR #14 Adds the graph schemaextension get cmd #14 Mar 23, 2019
@coveralls
Copy link

coveralls commented Mar 23, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling 8467396 on ypcode:dev-schemaextension-get2 into b4bf46b on pnp:dev.

@waldekmastykarz
Copy link
Member

waldekmastykarz commented Mar 23, 2019

Thank you @ypcode! We'll review it shortly

@VelinGeorgiev VelinGeorgiev self-assigned this Mar 25, 2019
Copy link
Contributor

@VelinGeorgiev VelinGeorgiev left a comment

Hi @ypcode , almost perfect command!

I've added minor comments in the code. Could please have a look?

Could you please amend you commit comment to include the issue number? You can change your current comment Adds graph schmeaextension get command using a git command like:
git commit --amend -m "Adds 'graph schemaextension get' command solving #14"

Utils.restore([
vorpal.find,
request.get,
request.post
Copy link
Contributor

@VelinGeorgiev VelinGeorgiev Mar 25, 2019

Choose a reason for hiding this comment

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

Could you please remove the request.post? It is no used at all.

Copy link
Contributor

@hugoabernier hugoabernier Mar 25, 2019

Choose a reason for hiding this comment

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

Also, it should be schemaextension not schmeaextension

});
});


Copy link
Contributor

@VelinGeorgiev VelinGeorgiev Mar 25, 2019

Choose a reason for hiding this comment

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

Could you please add an extra test to test /schemaExtensions endpoint in a case of failure?

For example schema not found response.
Failure response for schema not found is with status code 404:

{
    "error": {
        "code": "ResourceNotFound",
        "message": "Cannot find resource with id: 123.",
        "innerError": {
            "request-id": "4827c0cb-3580-4e93-98db-cbac18e9951f",
            "date": "2019-03-25T14:09:57"
        }
    }
}

Copy link
Contributor

@hugoabernier hugoabernier Mar 25, 2019

Choose a reason for hiding this comment

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

Same here, the path should be schemaextension not schmeaextension

}

const requestOptions: any = {
url: `${auth.service.resource}/v1.0/schemaExtensions/${args.options.id}`,
Copy link
Contributor

@VelinGeorgiev VelinGeorgiev Mar 25, 2019

Choose a reason for hiding this comment

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

Could you please escape the user input for id in the url?

${auth.service.resource}/v1.0/schemaExtensions/${args.options.id} can be changed to
${auth.service.resource}/v1.0/schemaExtensions/${encodeURIComponent(args.options.id)}

@VelinGeorgiev
Copy link
Contributor

VelinGeorgiev commented Apr 13, 2019

@ypcode , by any chance, have you looked at me comments. What do you think?

@ypcode
Copy link
Contributor Author

ypcode commented Apr 14, 2019

@waldekmastykarz
Copy link
Member

waldekmastykarz commented May 12, 2019

Closing this for now due to lack of response/progress. Let's reopen when active again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants