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

Add tags #2

Merged
merged 4 commits into from
Sep 3, 2017
Merged

Add tags #2

merged 4 commits into from
Sep 3, 2017

Conversation

vincentvanderweele
Copy link
Contributor

As described in #1, I added support for adding tags to all endpoints of a specific service, like this:

{
  "swagger": "2.0",
  "info": {
    "title": "Swagger Combine Rename Example",
    "version": "1.0.0"
  },
  "apis": [
    {
      "url": "http://petstore.swagger.io/v2/swagger.json"
    },
    {
      "url": "https://api.apis.guru/v2/specs/medium.com/1.0.0/swagger.yaml",
      "tags": {
        "add": [
          "Medium"
        ]
      }
    }
  ]
}

There are of course two places where tags are used: in the schema root and in every operation. I chose to only add these tags to the operation tags because the root tags will all be merged into a single array anyway (so it doesn't make much sense to allow adding tags per service).

I don't handle the case that an added tag already exists because renameTags doesn't handle that either. I also chose to not add a new integration test for this functionality but this can of course be added if needed.

@fabsrc fabsrc self-requested a review September 1, 2017 14:46
@fabsrc
Copy link
Contributor

fabsrc commented Sep 1, 2017

Hi Vincent,

thank you for your contribution 😊 This feature really is a great addition to this module.
However there are two things you could improve in your PR:

  • Add some documentation to the README.md file
  • Check and remove duplicate values in the tags array (e.g. by using lodash's uniq function). If you want to you can do this for the renameTags function as well.

Integration tests are optional, maybe we will add some later.

@fabsrc fabsrc removed their request for review September 1, 2017 14:51
@vincentvanderweele
Copy link
Contributor Author

Will do!

Currently I have a problem that an array gets converted to an object with integer keys. I'm not sure if my change is causing this, but if it is, I'll obviously fix that as well in this PR.

@vincentvanderweele
Copy link
Contributor Author

  • fixed the bug and added an extra testcase (earlier I forgot to check if we were really dealing with an operation object, so it could also be the shared parameters).
  • added an example to the readme and made that same example an integration test
  • removed duplicate tags in renameTags and addTags and added testcases for that

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