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(core): add documentUrl to JS api and cli formatters #2443

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

Conversation

dweber019
Copy link
Contributor

@dweber019 dweber019 commented Mar 29, 2023

Fixes #1488.

Checklist

  • Tests added / updated
  • Docs added / updated

Does this PR introduce a breaking change?

  • Yes
  • No

Screenshots
CleanShot 2023-03-29 at 23 51 42@2x
CleanShot 2023-03-29 at 23 52 26@2x

All formatters have been updated.

@dweber019
Copy link
Contributor Author

Should we add the docuemtnUrl type here stoplightio/types#127?

@nahteb
Copy link

nahteb commented Mar 30, 2023

thanks for raising this PR, we've just recently added documentationUrls to our ruleset and would find this really useful 👍

@dweber019
Copy link
Contributor Author

@magicmatatjahu @smoya @jonaslagoni any update on this?

@jonaslagoni
Copy link
Collaborator

I dont get why we are assigned tbh. @P0lip this is a core change, while it does change something for the AsyncAPI rules, this is all you 😄

@jonaslagoni jonaslagoni requested a review from P0lip April 25, 2023 18:38
@mnaumanali94
Copy link
Contributor

👋🏼 @dweber019 First of all, I would like to apologize for the delay in responding to your pull request. We appreciate your valuable contribution to the project.

We understand your intention to output the documentation URL as part of the lint process. However it may clutter the output and become repetitive. Additionally, not all organizations utilize the documentation URL, and some prefer using the description field.

With that in mind, we would like to propose a couple of alternative options:

Add the functionality to output the documentationURL behind the --verbose flag. This would make it an opt-in feature, giving users the choice to include the documentation URL in their lint output. The downside to this approach is that it may not cater to organizations that use the description field instead of the documentationURL.
Introduce a new spectral ruleset explain command that displays information for each rule, including severity, documentationURL, and description. This would provide a more comprehensive overview of the rules without cluttering the lint output.
We would love to hear your thoughts on these approaches or any other ideas you might have. Once we have your input, we can work together to make the necessary changes and integrate them into the project.

Thank you once again for your contribution, and we look forward to collaborating with you on this feature.

@dweber019
Copy link
Contributor Author

Hi @mnaumanali94

Thx for the feedback. I agree with you. It depends on the use case.
How would you imagine this explain command.

  1. Is this a options flag on lint, and if a rule fails more output is generated
  2. or is it a standalone command to basically extract rule information from the rules file without linting?

For our use case, the first one would satisfy our needs and makes IMHO more sense, as the documentation url or description is needed in case of error mostly on CI side, where you don't like to rerun a command to understand the error.

@P0lip P0lip force-pushed the develop branch 7 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
@shelleeboo
Copy link

@dweber019 Thanks you for raising this issue and for your contribution. I have a similar requirement to make use of the documenationUrl property in Javascript.

@mnaumanali94 Any idea if this PR will be accepted and merged anytime soon?

Cheers!

@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 2 times, most recently from 1a04f25 to dc90b7a Compare April 4, 2024 13:19
@mnaumanali94
Copy link
Contributor

Is this a options flag on lint, and if a rule fails more output is generated

I would lean towards this

@johannesmarx
Copy link
Contributor

johannesmarx commented May 24, 2024

@dweber019 thanks for taking care of this feature as I was also about to raise a similare request.
Is there any reason, why no all formatters are considered?

IMHO also github-actions would heavily benefit from it as users simply can follow the docmentation links in GitHub pull requests.

Thanks

@dweber019 dweber019 force-pushed the feat/expose-documentation-url branch from 9200f3d to 515f6b0 Compare May 30, 2024 21:14
@dweber019
Copy link
Contributor Author

@dweber019 thanks for taking care of this feature as I was also about to raise a similare request. Is there any reason, why no all formatters are considered?

IMHO also github-actions would heavily benefit from it as users simply can follow the docmentation links in GitHub pull requests.

Thanks

No reason, github actions was not implemented when I last committed.

I'm just started to update this pr but have some harness binary issues.

@dweber019 dweber019 force-pushed the feat/expose-documentation-url branch 2 times, most recently from 598876e to 2d7ae95 Compare May 30, 2024 22:38
@dweber019 dweber019 requested a review from a team as a code owner May 30, 2024 22:38
@dweber019 dweber019 requested review from ryotrellim and removed request for P0lip and magicmatatjahu May 30, 2024 22:38
@dweber019
Copy link
Contributor Author

dweber019 commented May 30, 2024

harness could be solved by using node 18.
I updated now everything and change the whole feature to a CLI flag.
Additionally, github-actions and sarif is included now.

@mnaumanali94 could you do a review?

@dweber019 dweber019 force-pushed the feat/expose-documentation-url branch from 6e15f43 to 08ba0bc Compare June 10, 2024 19:22
@mnaumanali94
Copy link
Contributor

LGTM!

mnaumanali94
mnaumanali94 previously approved these changes Jul 11, 2024
@dweber019
Copy link
Contributor Author

@jonaslagoni @smoya could someone of you check this pr. Thanks a lot

Copy link
Collaborator

@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.

@jonaslagoni @smoya could someone of you check this pr. Thanks a lot

As mentioned earlier, no need for us to review core changes that affects AsyncAPI rules or outputs of it 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

make documentationUrl available as part of Javascript API linting result
6 participants