Skip to content

Conversation

@rbordeanu
Copy link
Contributor

remove special chars in swagger 'operationId', because this property is used in code generation tools for method names, and it can lead to invalid code

Types of changes

What types of changes does your code introduce?
Put an x in the boxes that apply

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Proposed changes

Describe the nature of this change. What problem does it address?

Checklist

Put an x in the boxes that apply

  • I have read the contribution guidelines
  • For non-bugfix PRs, I have discussed this change on the mailing list/slack team.
  • I have run grunt to verify the unit tests pass
  • I have added suitable unit tests to cover the new/changed functionality

remove special chars in swagger 'operationId', because this property is used in code generation tools for method names, and it can lead to invalid code
@coveralls
Copy link

coveralls commented Feb 22, 2018

Coverage Status

Coverage increased (+0.3%) to 73.789% when pulling aad7159 on rbordeanu:pr/swagger-sanitize-operationId into f5729a2 on node-red:master.

@kazuhitoyokoi
Copy link
Member

@rbordeanu Thank you for your pull request. I'm not familiar with your regular expression, "/(?!\w|\s)./g". In the following cases which you mentioned on Slack, I think that "/\W/g" is better because it's simple and can cover many cases.

/accounts___GET_authorization
/accounts___GET_psuIpAddress

Could you give me another example of operationId if yours is better?

@rbordeanu
Copy link
Contributor Author

@kazuhitoyokoi It uses the negative lookahead pattern.
You can see a breakdown of the regex here: https://regex101.com/r/RSmDUN/1/
From my experience in general it's better to whitelist (in this case, words and whitespaces) than to exclude. ;) But I can change it to your suggestion if you prefer, in the end they basically do the same thing.

@kazuhitoyokoi
Copy link
Member

@rbordeanu Thank you for your sharing the web site. I learned it! :)
@HiroyasuNishiyama Could you merge the pull request?

@HiroyasuNishiyama HiroyasuNishiyama merged commit 69ba8f9 into node-red:master Feb 26, 2018
@kazuhitoyokoi
Copy link
Member

@HiroyasuNishiyama Thank you. 👍

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.

4 participants