bootprint / bootprint-openapi

Bootprint-module to render OpenAPI specifications, this repository has moved to https://github.com/bootprint/bootprint-monorepo/tree/master/packages/bootprint-openapi
MIT License
357 stars 48 forks source link

Fixes #65 #66

Closed aberman closed 8 years ago

aberman commented 8 years ago

This will allow markdown in the security definitions description field.

nknapp commented 8 years ago

Some checks have failed, but on the first glance, it does not seem to be your changes that caused it. I'll have a look into it.

aberman commented 8 years ago

Looks like the petstore test failed. I was able to confirm locally as well.

nknapp commented 8 years ago

There is an issue here

nknapp commented 8 years ago

I think the failing tests are related to the changes in this commit. The security definitions partial appears in the error stack-trace. Maybe the fix is not as easy as that.

aberman commented 8 years ago

Yeah, I was looking at it and I think the issue has to do with the scopes. They're currently coming out as [Object] which could be the issue. I think the template needs to be rewritten to support all the features of the security definitions properly. I'll look into it.

nknapp commented 8 years ago

I remember: The security definitions is one part that is actually still broken or at least only working a little: The partial swagger/securityDefinitions.hbs just print all key-value-pairs except the type: (see here), so your change actually added markdown to all values, not just the description. The correct fix (and my hint in #62 was misleading here) is to write an explicit statement for all keys defined in the OpenAPI-specification.

Maybe similar to the parameters.hbs.

nknapp commented 8 years ago

If you want to reorganize the whole security section, go ahead. This is actually issue #7 and it has been there for a long time. This seems to be a bit more work though. I'm not sure about how to create a good layout for the fields. Something that doesn't waste space, but isn't to crowded either.

nknapp commented 8 years ago

I'll give you push access, so you can work in a branch on the project. This might make it easier to collaborate and it lets Travis run on every push

nknapp commented 8 years ago

Oh, and in case you wonder: Most labels are defined here and attached to classes via :before {} CSS-selectors.

This was my way to allow injecting translations (since you can override CSS easily with a configuration file), but I think there are better way to handle this, so something like this would be perfectly fine:

<section class="sw-security-properties">
    <dt><span class="security-property-name">type:</span></dt>
    <dd><span class="security-property-type">{{type}}</span></dd>

    <dt><span class="security-property-name">description:</span></dt>
    <dd><span class="security-property-type">{{md description}}</span></dd>

    {{#ifeq type 'apikey'}}
        <dt><span class="security-property-name">name:</span></dt>
        <dd><span class="security-property-type">{{name}}</span></dd>
    {{/ifeq}}
    ...
    ...
    ...
</section>
aberman commented 8 years ago

It should work now. I didn't make it too pretty to be honest. I just listed things using a dt-dl-dd. It looks fine to me with my swagger docs. I didn't play around with any of the less files to spruce things up.

nknapp commented 8 years ago

Looks great (I haven't looked at the output though, but the template looks just). I would just omit the closing and opening '

`-tags in the middle of the list. Better

<dl>
  <dt>label</dt><dd>description</dd>
  <dt>label</dt><dd>description</dd>
  <dt>label</dt><dd>description</dd>
</dl>

than

<dl>
  <dt>label</dt><dd>description</dd>
</dl>
<dl>   
  <dt>label</dt><dd>description</dd>
</dl>
<dl>
  <dt>label</dt><dd>description</dd>
</dl>

After that, you can merge the PR using the "Merge Pull Request"-button. It should offer you to create a new commit message for squashed commit of the branch. When you write the commit message, please have a look here for the commit message format. The first line will appear in the change-log.

And thank you very much for you efforts.

aberman commented 8 years ago

Ah, good catch. Didn't mean to do that. Copy-paste error. I'll fix it up.

nknapp commented 8 years ago

Something has gone wrong with the commit comment during the merge. (The first line is now "Merge pull request #66 from aberman/master") But I have published the package now anyway and adapted the changelog manually. Thanks for your work

Closes #7 as well.

aberman commented 8 years ago

That sucks. I definitely put in a full comment following the guidelines you pointed me to. Sorry for that. Not sure what I did wrong.

aberman commented 8 years ago

If you look here, you can see my commit message: https://github.com/bootprint/bootprint-openapi/commit/31de1ba7cc0331d731abf946f2352055f5d1659c

nknapp commented 8 years ago

I'm sorry. I probably wasn't very clear. I have made my last comment early in the morning, shortly before going to work.

I think you have been writing a very good commit message. What I meant with "something went wrong" was: git log shows the commit message as

Merge pull request #66 from aberman/master

Improved the look of the security definitions which now properly converts markdown to HTML in the description field.

Closes #65
- Added markdown conversion to description field
- Rewrote the security definitions template to make it a bit prettier
- Added a new test to confirm the markdown conversion is occurring properly on the security definition description field

I would have expected the message to start with Improved the look.... But I can see several causes for this and I have to check it out myself.

However, I think it is not a big deal. All of this is still a learning process to me. I'm very glad that you accepted the invitation to the team and that you made the effort to follow all the rules I set up. And I am glad that you merged the PR yourself.

Having people helping in the project means more to me than having 100% perfect commit messages.