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

initial PR for raml1.0 support #248

Closed
wants to merge 1 commit into from
Closed

initial PR for raml1.0 support #248

wants to merge 1 commit into from

Conversation

sichvoge
Copy link
Contributor

@sichvoge sichvoge commented Aug 5, 2016

Since 2.5.0 introduced a lot of changes and conflic resolution when merging develop back to raml1.0 was almost impossible, @kevinrenskers asked to basically create a new branch and apply all the changes. This PR does that. Several notes here:

  • I had to change the ramlObj.securitySchemeWithName function back using a for instead of find since it was not working.
  • There are some displaying issues (see images below)

Can everyone who participated updating the RAML 1.0 branch check this again please and give a LGTM? (cc/ @ivokh @edentsai @jerrobertson)

It was quite some work and I really hope I did not miss anything. The tests that I have run look good to me.

Displaying issues:

Here the rendering for the second property that is also a type is really strange - it should be a bullet point

issue_1

Should we really display Type: if there is no type.

issue_2

@kevinrenskers
Copy link
Member

Is this ready for a PR? How far along is the RAML 1.0 support? It was my understanding that it also depends on changes in raml2obj?

@@ -67,7 +68,144 @@ function getDefaultConfig(mainTemplate, templatesPath) {

// Add extra function for finding a security scheme by name
ramlObj.securitySchemeWithName = function (name) {
return ramlObj.securitySchemes.find((scheme) => scheme[name]);
for (var index = 0; index < ramlObj.securitySchemes.length; ++index) {
Copy link
Member

Choose a reason for hiding this comment

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

We can do better then a for loop, at the very least we can use ramlObj.securitySchemes.forEach. But since all it does it find something that matches and then returns it, really the .find should work. If it isn't, let's debug & fix that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It isn't. That's why I removed it.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah so there was a bug somewhere, because it should :) I'll look into it today.

Copy link
Member

Choose a reason for hiding this comment

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

A version that works, return the value of the scheme object, rather then the object itself. I already fixed it in develop by the way.

    ramlObj.securitySchemeWithName = function (name) {
        const result = ramlObj.securitySchemes.find(s => s[name]);
        if (result) {
          return result[name];
        }
        return {};
      };

@kevinrenskers
Copy link
Member

kevinrenskers commented Aug 5, 2016

There seem to be a lot of added logic in raml2html now (like getRootType, isChildType, propertiesOfType), wondering if that doesn't belong in raml2obj rather? They can be exported as helper functions, or even modify the raml object already itself. It just doesn't seem like it belongs in the renderer, but rather the processor library (raml2obj).

There also seems to be some code styling issues, can you run npm run lint? Thanks!

@sichvoge
Copy link
Contributor Author

sichvoge commented Aug 5, 2016

Sure! And I agree, most of it will be important for both md and html. It's applied to the raml2obj object anyways.

@kevinrenskers
Copy link
Member

I've taken another look at this PR after fixing the securitySchemeWithName function (in the develop branch). I really don't think this should be merged. All these extra functions just don't belong in raml2html, they don't use ES6 array and object functions (map, filter, reduce), and it doesn't lint.

Let's not use any imperative for loops. Especially propertiesOfType needs to be refactored.

I'm going to make the first step by moving securitySchemeWithName to raml2obj, the other similar functions should then follow.

@naterkane
Copy link

what's the target version of node on this branch?
i've been running into some issues where request body examples in JSON are rendering as [Object object] and it appears to be on this level and not the fault of raml2obj

@kevinrenskers
Copy link
Member

This branch needs Node 4.0, just like the develop branch. The released version (master brach, 2.5.0) still works on older versions of Node.

@naterkane
Copy link

Good to know. Thanks.

On Aug 9, 2016 1:39 PM, "Kevin Renskers" notifications@github.com wrote:

This branch needs Node 4.0, just like the develop branch. The released
version (master brach, 2.5.0) still works on older versions of Node.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#248 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AABVqkIda-B2z6KLp45ImBtOs1Guns5Aks5qeLtogaJpZM4JdqJN
.

@sichvoge
Copy link
Contributor Author

@kevinrenskers would you take the opportunity and move those functions to ramlobj? I can do that, but only have time during the weekend to work on that.

@kevinrenskers
Copy link
Member

Hey @sichvoge,

I've published raml2obj 4.0.0-beta1 with the RAML 1.0 support changes, and raml2html 4.0.0-beta1 which uses that new version of raml2obj.

I have not copied over the template changes, not the helper functions needed for that, because it just makes more sense to do that while working on the new template. So in my opinion we can close this PR, delete this branch (as well as the other RAML 1.0 branch), and concentrate on building the new template (which will support 0.8 and 1.0 files).

What do you think?

@kevinrenskers
Copy link
Member

What I need from the RAML Workgroup, or someone from the community, is awesome example files for both RAML 0.8 and 1.0 which include all the features that raml2html needs to support. I can then see how raml2obj needs to change to make it easier for clients such as raml2html and raml2md to work with a unified format instead of a bunch of if statements in the templates. That will make it 100x easier to write new templates because then we only have to care about one JS object format, so to speak.

In fact, I'll create a new issue with the progress so far, what I need to move forward, etc.

@kevinrenskers
Copy link
Member

I'm closing this PR, please check out #254 because I think we should completely remove the raml1.0_new branch and change our focus.

@kevinrenskers kevinrenskers deleted the raml1.0_new branch August 15, 2016 07:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants