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

Integrate sdk generator #1429

Merged
merged 9 commits into from
Aug 30, 2016
Merged

Integrate sdk generator #1429

merged 9 commits into from
Aug 30, 2016

Conversation

marla-singer
Copy link
Contributor

@marla-singer marla-singer commented Aug 22, 2016

add modal form, add button "SDK Generator", add list of language

Closes #1376

@bajiat
Copy link
Contributor

bajiat commented Aug 22, 2016

@marla-singer Is this PR ready for review?

@marla-singer
Copy link
Contributor Author

@bajiat Yes

@marla-singer
Copy link
Contributor Author

@bajiat Now the file is downloaded slow because of the cross domain request. I suggest to add a spinner for user's usability

@brylie
Copy link
Contributor

brylie commented Aug 22, 2016

@marla-singer that sounds like a good enhancement to add user feedback. Will you include it in this PR?

@marla-singer
Copy link
Contributor Author

@brylie yes, sure

@bajiat bajiat added the WIP label Aug 22, 2016
@bajiat
Copy link
Contributor

bajiat commented Aug 22, 2016

@marla-singer I suggest that we add a Work in progress (WIP) label to this PR until the spinner has been added. Please remove the label when you are done with adding the spinner and the PR is ready for review.

@marla-singer marla-singer removed the WIP label Aug 22, 2016
@@ -12,3 +12,7 @@ iframe {
.s-alert-box {
z-index: 10000
}

button#sdk-code-generator {
margin-right: 10px
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use relative values for layout: em, rem, or %

@brylie brylie added the WIP label Aug 23, 2016

Template.sdkCodeGeneratorModal.onDestroyed(function () {
// Unset session
Session.set('currentDocumentationFileURL', undefined);
Copy link
Contributor

@brylie brylie Aug 23, 2016

Choose a reason for hiding this comment

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

Try to use template instance variables rather than Session variables. You can add an instance variable like so:

// Inside of Template lifecycle callback
const instance = this;

instance.variableName = "value";

Then you can use the instance variable inside of Template events and helpers. Use the ReactiveVar package, if you need the instance variable to be reactive.

// inside of instance lifecycle callback
const instance = this;

// Create reactive variable
instance.someReactiveVariable = new ReactiveVar();

// set the reactive variable value
instance.someReactiveVariable.set("value");

// get the reactive variable value
instance.someReactiveVariable.get("value");

In order for the above suggestions to work, it may be necessary to move the modal markup into the parent template. That way the instance variables will be availabble inside the template events and helpers.

Otherwise, you can consider calling the Codegen instance from within this template's 'click button' event, or similar.

@brylie
Copy link
Contributor

brylie commented Aug 23, 2016

When submitting the petstore swagger example, I get the following errors in Chrome console:

POST https://generator.swagger.io/api/gen/clients/python 400 (Bad Request)
POST https://generator.swagger.io/api/gen/clients/android 400 (Bad Request)

For clarification, I have downloaded the petstore example and uploaded it via the Manage documentation modal.

@brylie
Copy link
Contributor

brylie commented Aug 23, 2016

Can you please provide an example Swagger file, so that we can test using the same file?

@brylie
Copy link
Contributor

brylie commented Aug 23, 2016

When using the swagger petstore example file we are also getting file validation errors, which may be incorrect:

screenshot from 2016-08-23 13-01-37

@brylie
Copy link
Contributor

brylie commented Aug 23, 2016

Overall, this pull request looks good. The user interface is nice, and the code is easy to read. I have made some points about code structure and conventions, which hopefully won't add too much more work.

@marla-singer
Copy link
Contributor Author

@brylie thanks for you points! I fixed some remarks.

I think the problem with 400 request happens on localhost.
You can test SDK codegen with swagger petstore example file in this page
Before I used this standard example both JSON and YAML format

@bajiat
Copy link
Contributor

bajiat commented Aug 24, 2016

@marla-singer @brylie What if you get the list dynamically and then have a function/method that replaces the names to a more user friendly name? If the list available from Codegen server has changed and there is no corresponding name in your function, then it will be displayed as is (= as retrieved from server)?

@marla-singer
Copy link
Contributor Author

@bajiat Allright, I've got it

implement dynamicall list and add friendly name
@marla-singer
Copy link
Contributor Author

The task can be tested on this page

@brylie
Copy link
Contributor

brylie commented Aug 25, 2016

@marla-singer the hosted UI works good. We will demonstrate it briefly at our meeting this morning.

urlParameters = response;

// Create list of friendly language names
LanguageList = [];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@brylie I used global variables in the file collection.js (LanguageList, urlParameters) But I noticed you have done a restructure project in pull request #1437, where it is necessary to use export/import in the collection files. I can't use import because of the async callback in the GET request that returns data.
Now the data processing from Swagger is executed in main file (codegenerator.js) instead of collection.js and Modal form is waiting for data from the callback before displaying a dropdown list with the languages. I added a spinner, because it is not immediately

Can you please give advice to improve this solution?

@brylie
Copy link
Contributor

brylie commented Aug 26, 2016

@marla-singer what import do you need? Is it a collection that you cannot import? What file do you mean, where the import will not work?


import { LanguageParams, LanguageList, DocumentationFiles } from '/documentation/collection/collection';
import { DocumentationFiles } from '/documentation/collection/collection';

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@brylie File codegenerator.js
If I do import the variables 'LanguagesList' and 'LanguageParams' from collection.js, this variables wil be undefined.

@brylie
Copy link
Contributor

brylie commented Aug 26, 2016

You could, for now, just define the variables within the file where they are used.

@marla-singer
Copy link
Contributor Author

Then the code is ready for reviewing

// The method is to replace dash on space and to do the capital letter of each word
// Some language names can't be convert by standard method.
// Variable 'languageHashName' keeps specific names
const languageHashName = {
Copy link
Contributor

@brylie brylie Aug 26, 2016

Choose a reason for hiding this comment

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

Move this to a different file. The only purpose of the collection.js file is to define and export the collection.

This collection is specifically for storing swagger documents, which may be confusing here. Codegen code should only be defined within the codegen module:

documentation/codegenerator/client/codegenerator.js

@marla-singer
Copy link
Contributor Author

marla-singer commented Aug 26, 2016

I should add this constant to codegenerator.js in part .onCreated or keep it in different file?

@brylie
Copy link
Contributor

brylie commented Aug 29, 2016

It might be a good idea to keep it in a separate file. E.g. codegenarator_languages.js

@brylie
Copy link
Contributor

brylie commented Aug 29, 2016

All other code here looks good, and we can merge these changes once the codegenerator languages file/import is working.

// The method is to replace dash on space and to do the capital letter of each word
// Some language names can't be convert by standard method.
// Variable 'languageHashName' keeps specific names
const languageHashName = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Keep this in the same file as the language names. E.g. codegenerator_languages.js

@brylie
Copy link
Contributor

brylie commented Aug 30, 2016

Just checking on this PR. We can merge it if the files are in the desired location. E.g. hash names and language strings. Is hashName.js the desired file name for the hash lookup map?

@marla-singer
Copy link
Contributor Author

I renamed the file and variable and gave more clearly names. Another files are in the desired location

@brylie brylie merged commit 31efc14 into develop Aug 30, 2016
@brylie brylie deleted the feature/integrate-sdk-download branch August 30, 2016 07:56
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

3 participants