search

stoplightio / spectral

A flexible JSON/YAML linter for creating automated style guides, with baked in support for OpenAPI (v3.1, v3.0, and v2.0), Arazzo v1.0, as well as AsyncAPI v2.x.
https://stoplight.io/spectral
Apache License 2.0
2.45k stars 234 forks source link

feat(formatters): move formatters to a separate package #2468

Closed sennyeya closed 1 year ago

sennyeya commented 1 year ago

Fixes #2467. Moves the formatters to their own package that is then imported by @stoplight/spectral-cli.

👀 @P0lip

Checklist

Existing tests should cover any issues, this PR is just a move with additional dependency work.

Does this PR introduce a breaking change?

The formatters for the CLI have been pulled out to their own package, not necessarily breaking for any contracts, but will require pulling in a new package on CLI version update.

Additional context

I wasn't sure if this should sit in packages/core or a separate package, I think it makes sense to pull it completely out as it's not core functionality and just a presentation layer, but I could go either way.

sennyeya commented 1 year ago

Couple of questions:

  1. What is the commit message supposed to look like? Not sure I'm following from the error message.
  2. Is there somewhere that the memfs is populated with the HTML files during Karma tests, getting a not found error locally and in the CI, but not sure where that would be solved?
sennyeya commented 1 year ago

@P0lip Any ideas on why the Karma tests are failing? I think it's related to memfs mocks, but it seems odd that moving the directory causes this issue in the same test case.

P0lip commented 1 year ago

Yeah, that is most likely due to the HTML formatter relying on fs and it not being available in Karma launcher we use (it's plain Chrome browser in headless mode, we use webpack to bundle things and have some basic fs mock). I'll try to update HTML formatter to not depend on fs at all so that we don't need to worry about anything.

P0lip commented 1 year ago

2474 - WIP

sennyeya commented 1 year ago

Great, thanks!

P0lip commented 1 year ago

@sennyeya could you merge the latest changes when you have a minute? 🙏 that should unblock test-browser 🤞

sennyeya commented 1 year ago

@P0lip It looks like cliui is having issues with the Karma browser tests, which I think is expected as it's just a nodejs module. I think this was excluded originally by including packages/ci/** in the karma.config.ts exclude section. As you'll be updating this package to be all environment friendly, I think it makes sense to skip this for now.

Other than that, everything should be good minus the commit message (what's the format for those?)

P0lip commented 1 year ago

Other than that, everything should be good minus the commit message (what's the format for those?)

it has to be accepted by commitlint per its config here https://github.com/stoplightio/spectral/blob/develop/commitlint.config.js

P0lip commented 1 year ago

I'll take a final look on Monday, and if it's all good I will proceed and merge it. Thanks a lot of your time and contribution. Appreciate it a lot.

sennyeya commented 1 year ago

@P0lip Thanks for your help pushing this through!

stoplight-bot commented 1 year ago

:tada: This PR is included in version 6.7.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

stoplight-bot commented 1 year ago

:tada: This PR is included in version 1.18.1 :tada:

The release is available on npm package (@latest dist-tag)

Your semantic-release bot :package::rocket: