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(rulesets): add rule to check if messageId is defined #2281

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

magicmatatjahu
Copy link
Contributor

@magicmatatjahu magicmatatjahu commented Sep 16, 2022

Checklist

  • Tests added / updated
  • Docs added / updated

Does this PR introduce a breaking change?

  • Yes
  • No

Additional context

  • change severity of asyncapi-operation-operationId rule from error to warning
  • add asyncapi-message-messageId rule, with similar functionality like asyncapi-operation-operationId
  • add unit tests
  • update docs
  • add more formats (needed by new rule)
  • merge traits before checking ids.

cc @jonaslagoni @smoya

@magicmatatjahu magicmatatjahu added enhancement New feature or request AsyncAPI Issues related to the AsyncAPI ruleset labels Sep 16, 2022
@magicmatatjahu magicmatatjahu changed the title feat(rulesets): rule to check if messageId is defined feat(rulesets): add rule to check if messageId is defined Sep 16, 2022
@magicmatatjahu
Copy link
Contributor Author

magicmatatjahu commented Sep 19, 2022

I have to update that PR to handle also cases with traits. For messageId and operationId.

Copy link
Collaborator

@smoya smoya left a comment

Choose a reason for hiding this comment

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

💯 👍

I +1 to the change. I'm concerned about the possible breaking change this could become.

If any user relies on the operationId for a critical feature, this change will possibly make their apps to fail.
I believe this should be considered a breaking change, but let's see what other maintainers say.

@P0lip
Copy link
Contributor

P0lip commented Sep 26, 2022

Yeah, I'd say it's a breaking change indeed.
As such, we could release spectral-rulesets 2.x.x if you're okay with.
I don't have any issue with it.

@magicmatatjahu
Copy link
Contributor Author

@smoya @P0lip I'm not a fan to release version 2.0.0 rulesets with such a case. As there will be much bigger changes then we will change error to warning, now I reverted my changes regarding operationId. Please check again, thanks!

@@ -297,7 +303,7 @@ channels:

### asyncapi-operation-operationId

This operation ID is essentially a reference for the operation. Tools may use it for defining function names, class method names, and even URL hashes in documentation systems.
Each Operation must have an "operationId" field defined. Tools can use it to define function names, class method names and even URL hashes in documentation systems.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Each Operation must have an "operationId" field defined. Tools can use it to define function names, class method names and even URL hashes in documentation systems.
Each Operation must have an "operationId" field defined. Tools may use it to define function names, class method names, and even URL hashes in documentation systems.

@P0lip P0lip force-pushed the develop branch 3 times, most recently from dc9d7f4 to 44c40e2 Compare May 23, 2023 22:56
@P0lip P0lip force-pushed the develop branch 4 times, most recently from 02ec0d4 to 84faec8 Compare June 9, 2023 19:43
@P0lip P0lip force-pushed the develop branch 3 times, most recently from 9e92f34 to 6d09915 Compare September 20, 2023 18:42
@P0lip P0lip force-pushed the develop branch 3 times, most recently from dc90b7a to c22f408 Compare April 4, 2024 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AsyncAPI Issues related to the AsyncAPI ruleset enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants