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.

Sign up for GitHub

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

Jump to bottom

fix: config option for operationId naming convention #149

Merged
merged 2 commits into from
Mar 4, 2020
Merged

fix: config option for operationId naming convention #149

merged 2 commits into from
Mar 4, 2020

Conversation

presto21
Copy link
Contributor

@presto21 presto21 commented Feb 27, 2020 •
edited
Loading

Naming conventions for operationId should be configurable.

Change:

  • added config option to README.md and .defaultsForValidator.js
  • added config logic to operation-ids.js
  • added a test case to exercise the new behavior

Related: Issue 148

@codecov
Copy link

codecov bot commented Feb 27, 2020 •
edited
Loading

Codecov Report

Merging #149 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #149   +/-   ##
=======================================
  Coverage   93.17%   93.17%           
=======================================
  Files          61       61           
  Lines        2110     2110           
=======================================
  Hits         1966     1966           
  Misses        144      144

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a2e7011...e2c4a3e. Read the comment docs.

dpopp07
dpopp07 requested changes Feb 27, 2020
View reviewed changes
Copy link
Member

@dpopp07 dpopp07 left a comment

Choose a reason for hiding this comment

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

The approach is spot on, just have a few very minor tweaks for you to make

README.md Outdated Show resolved Hide resolved
src/plugins/validation/2and3/semantic-validators/operation-ids.js Outdated Show resolved Hide resolved
test/plugins/validation/2and3/operation-ids.js Outdated

const customRes = validate({ resolvedSpec }, customConfig);
expect(customRes.errors.length).toEqual(0);
expect(customRes.warnings.length).toEqual(0);
Copy link
Member

Choose a reason for hiding this comment

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

I appreciate the thought here, but these added checks are unnecessary. We can assume the message carrier logic works as it has it's own tests.

Also, just for future reference, it is best practice to test different behaviors in entirely different tests (different it functions), rather than adding checks for multiple behaviors within the same test.

@presto21 presto21 force-pushed the configOperationId branch 3 times, most recently from bf837c9 to e60434e Compare February 28, 2020 22:28
dpopp07
dpopp07 previously approved these changes Mar 3, 2020
View reviewed changes
Copy link
Member

@dpopp07 dpopp07 left a comment

Choose a reason for hiding this comment

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

Looks good! 👍

@dpopp07 dpopp07 dismissed their stale review March 3, 2020 22:51

Missed one thing

dpopp07
dpopp07 requested changes Mar 3, 2020
View reviewed changes
Copy link
Member

@dpopp07 dpopp07 left a comment

Choose a reason for hiding this comment

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

Changes look good but we need more documentation. I should have caught that in my previous review

README.md Show resolved Hide resolved
@presto21
fix: operationId should be configurable.
a2e7011 
- modified files in order to include a config option for operationId
@presto21 presto21 requested a review from dpopp07 March 3, 2020 23:33
dpopp07
dpopp07 approved these changes Mar 4, 2020
View reviewed changes
Copy link
Member

@dpopp07 dpopp07 left a comment

Choose a reason for hiding this comment

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

Looks good! 👍

@dpopp07 dpopp07 merged commit c8b07ff into master Mar 4, 2020
@dpopp07 dpopp07 deleted the configOperationId branch March 4, 2020 15:49
dpopp07 pushed a commit that referenced this pull request Mar 4, 2020
@semantic-release-bot
chore(release): 0.24.1 [skip ci]
a95b18a 
## [0.24.1](v0.24.0...v0.24.1) (2020-03-04)

### Bug Fixes

* make operationId naming convention check configurable ([#149](#149)) ([c8b07ff](c8b07ff))
@dpopp07
Copy link
Member

dpopp07 commented Mar 4, 2020

🎉 This PR is included in version 0.24.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dpopp07 dpopp07 dpopp07 approved these changes

Assignees
No one assigned
Labels
released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@presto21 @dpopp07