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

feat: extend the components model with has-like functions #192

Merged
merged 6 commits into from
Nov 19, 2020

Conversation

magicmatatjahu
Copy link
Member

@magicmatatjahu magicmatatjahu commented Nov 2, 2020

Description

As in description:

  • Extend Components model with has-like functions:
    • hasMessages
    • hasSchemas
    • hasSecuritySchemes
    • hasParameters
    • hasCorrelationIds
    • hasOperationTraits
    • hasMessageTraits
  • Extend Schema model with property(name) function to retrieve single property as Schema object -> idea from Add missing methods that help evaluate if given collection is empty or not #88 (comment)
  • Extend AsyncAPIDocument model with hasDefaultContentType() function
  • Add relevant unit tests
  • Add test-lib npm script for run only unit tests (without browser-test) - it's only for better dev-experience. Sometimes dev wants only write missing tests for models and do not want to run browser test every time -> in lib does not change anything itself.

Related issue(s)
Resolves #173

@magicmatatjahu magicmatatjahu added the enhancement New feature or request label Nov 2, 2020
@magicmatatjahu magicmatatjahu changed the title feat: Extend the components model with has-like functions feat: extend the components model with has-like functions Nov 4, 2020
@jonaslagoni
Copy link
Member

@magicmatatjahu is this still in progress or?

@magicmatatjahu
Copy link
Member Author

@jonaslagoni It's for the review :)

@jonaslagoni jonaslagoni self-requested a review November 16, 2020 12:46
Copy link
Member

@jonaslagoni jonaslagoni left a comment

Choose a reason for hiding this comment

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

@magicmatatjahu major question I have is the use of !! in components and some testing comments 🙇 Are you sure the use of double question marks provide the expected return?

test/models/asyncapi_test.js Outdated Show resolved Hide resolved
lib/models/components.js Show resolved Hide resolved
lib/models/components.js Show resolved Hide resolved
lib/models/components.js Show resolved Hide resolved
lib/models/components.js Show resolved Hide resolved
lib/models/components.js Show resolved Hide resolved
lib/models/components.js Show resolved Hide resolved
lib/models/components.js Show resolved Hide resolved
@magicmatatjahu
Copy link
Member Author

magicmatatjahu commented Nov 17, 2020

@jonaslagoni Thanks for review. I understand your point of view about returned value -> should return false if array/object is empty or true, but please check that in other models in very similar methods (like in PR), if field of given model is an empty array or object, then function returns true, like here https://github.com/asyncapi/parser-js/blob/master/lib/models/server.js#L61 only in case undefined or null function returns false. We must discuss in which way we should go with @derberg @fmvilas. Please note that changing default behavior will be breaking change :) I will add suggested tests. Thanks again!

@jonaslagoni
Copy link
Member

@jonaslagoni Thanks for review. I understand your point of view about returned value -> should return false if array/object is empty or true, but please check that in other models in very similar methods (like in PR), if field of given model is an empty array or object, then function returns true, like here https://github.com/asyncapi/parser-js/blob/master/lib/models/server.js#L61 only in case undefined or null function returns false. We must discuss in which way we should go with @derberg @fmvilas. Please note that changing default behavior will be breaking change :) I will added suggested tests. Thanks again!

Good point! What do you say we keep it as is for this PR and track it in another issue since it is, as you say, a major change?

@magicmatatjahu
Copy link
Member Author

@jonaslagoni Sure, good idea :) I want that @derberg and @fmvilas know about our problem as well.

@derberg
Copy link
Member

derberg commented Nov 18, 2020

yeah, I think we already talked about this in the past, in some other issue or PR, so we definitely need a followup issue as we just need to discuss and plan how we change all over the place. As far as I remember I was fan of what @jonaslagoni wrote, about treating empty as false, but recently when working on template-for-generator-templates I liked a lot has functions and was missing them from some models a lot. Apporach with has has advantage over other solution, but yeah, something to discuss elsewhere

@jonaslagoni
Copy link
Member

@magicmatatjahu will you or should I create the issue? Also just re-request review when you have added the corresponding tests 😄

@magicmatatjahu
Copy link
Member Author

@jonaslagoni I added missing tests only for checking returned empty object/array if given field doesn't exist in component or asyncapi model. Please check if everything is good :) If you want create issue, you're welcome 😄

jonaslagoni
jonaslagoni previously approved these changes Nov 19, 2020
Copy link
Member

@jonaslagoni jonaslagoni left a comment

Choose a reason for hiding this comment

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

Just a comment in regards to the titles for the tests. Not sure if its required to change that's up to you 😄 Good job 👍

test/models/asyncapi_test.js Outdated Show resolved Hide resolved
@sonarcloud
Copy link

sonarcloud bot commented Nov 19, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link
Member

@jonaslagoni jonaslagoni left a comment

Choose a reason for hiding this comment

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

👍

@derberg
Copy link
Member

derberg commented Nov 19, 2020

well done lads!

@derberg derberg merged commit 55aaa43 into asyncapi:master Nov 19, 2020
@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 1.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

jonastg added a commit to jonastg/parser-js that referenced this pull request Jan 18, 2021
* master: (24 commits)
  chore: Refactored code location for iterators (asyncapi#225)
  chore(release): 1.3.1 (asyncapi#222)
  fix: apply traits to standalone messages from components section (asyncapi#214)
  test: improve feedback loop from browser tests (asyncapi#216)
  ci: fix the space-issue in bump workflow (asyncapi#218)
  ci: update global workflows (asyncapi#217)
  chore(deps): bump ini from 1.3.5 to 1.3.7 (asyncapi#215)
  ci: rename pr testing job name and test node 14 (asyncapi#213)
  ci: bump workflow to start on push instead of release (asyncapi#212)
  chore(release): 1.3.0 (asyncapi#211)
  feat: add a traverse schema function (asyncapi#198)
  ci: add workflow that bumps parser in other asyncapi repos (asyncapi#208)
  ci: fix release workflow step that is responsible for handling twitter (asyncapi#209)
  ci: update global workflows (asyncapi#206)
  ci: disable any testing on draft PR (asyncapi#204)
  chore(release): 1.2.0 (asyncapi#202)
  feat: extend the components and asyncapi model with has-like functions (asyncapi#192)
  chore(deps-dev): bump semantic-release from 17.0.6 to 17.2.3 (asyncapi#199)
  chore(release): 1.1.1 (asyncapi#197)
  fix: channels with name '/' fail on validation (asyncapi#196)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extend the components model with has-like functions
4 participants