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

chore: set default severity for lint config #1292

Merged
merged 11 commits into from
Oct 19, 2023
Merged

Conversation

IgorKarpiuk
Copy link
Contributor

@IgorKarpiuk IgorKarpiuk commented Oct 10, 2023

What/Why/How?

Set default severity for the config validation and update warning message.
Add --lint-config option to all commands and exit with code 2 if config validation failed.
Previously, we had the default severity warn only for the lint command.

Reference

Related: #1277

Testing

Screenshots (optional)

Screenshot 2023-10-10 at 16 08 49

Check yourself

  • Code is linted
  • Tested with redoc/reference-docs/workflows (internal)
  • All new/updated code is covered with tests

Security

  • Security impact of change has been considered
  • Code follows company security practices and guidelines

@changeset-bot
Copy link

changeset-bot bot commented Oct 10, 2023

🦋 Changeset detected

Latest commit: bf41ff5

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@redocly/cli Minor
@redocly/openapi-core Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions
Copy link
Contributor

github-actions bot commented Oct 10, 2023

Command Mean [s] Min [s] Max [s] Relative
redocly lint packages/core/src/benchmark/benches/rebilly.yaml 1.261 ± 0.056 1.180 1.386 1.00
redocly-next lint packages/core/src/benchmark/benches/rebilly.yaml 1.288 ± 0.051 1.199 1.381 1.02 ± 0.06

@github-actions
Copy link
Contributor

github-actions bot commented Oct 10, 2023

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements 75.89% 4013/5288
🟡 Branches 65.78% 2138/3250
🟡 Functions 67.95% 651/958
🟡 Lines 76.07% 3761/4944
Show files with reduced coverage 🔻
St.
File Statements Branches Functions Lines
🟢
... / load.ts
88.24% (-3.6% 🔻)
78.57% (-1.92% 🔻)
100%
89.36% (-3.97% 🔻)
🟡 cli/src/utils.ts 61.64%
49.62% (-1.14% 🔻)
73.47% 60.93%
🟢
... / lint.ts
81.67% (-2.54% 🔻)
64.71% (-13.87% 🔻)
50%
83.05% (-2.66% 🔻)

Test suite run success

640 tests passing in 93 suites.

Report generated by 🧪jest coverage report action from bf41ff5

Comment on lines 115 to 122
it('should print warnign and error', () => {
printConfigLintTotals({ ...totalProblemsMock, warnings: 2 });
expect(process.stderr.write).toHaveBeenCalledWith(
'❌ Your config has 1 error and 2 warnings.\n'
);
expect(redColoretteMocks).toHaveBeenCalledWith('❌ Your config has 1 error and 2 warnings.\n');
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have only one rule or configuration, so there can only be one severity, error or warning

@IgorKarpiuk IgorKarpiuk self-assigned this Oct 11, 2023
@IgorKarpiuk IgorKarpiuk marked this pull request as ready for review October 11, 2023 07:18
@IgorKarpiuk IgorKarpiuk requested a review from a team as a code owner October 11, 2023 07:18
@tatomyr
Copy link
Contributor

tatomyr commented Oct 12, 2023

Does it make sense to exit with an error code if config linting fails with errors?

@lornajane
Copy link
Collaborator

I would expect an error to exit with a status code, yes

@IgorKarpiuk
Copy link
Contributor Author

IgorKarpiuk commented Oct 13, 2023

If we exit with an error when linting the config file (this applies to every command), do we need to add the --lint-config option for each command to specify the severity level for the configuration file?
@tatomyr @lornajane

@tatomyr
Copy link
Contributor

tatomyr commented Oct 13, 2023

do we need to add the -lint-config option for each command to specify the severity level for the configuration file?

@IgorKarpiuk this makes sense.
This wouldn't be a breaking change because we use warn as a default severity, right?

@IgorKarpiuk
Copy link
Contributor Author

This wouldn't be a breaking change because we use warn as a default severity, right?

@tatomyr Yes 🙂

@IgorKarpiuk IgorKarpiuk marked this pull request as draft October 16, 2023 12:28
@IgorKarpiuk IgorKarpiuk marked this pull request as ready for review October 17, 2023 14:41
'@redocly/cli': patch
---

Set default severity for the config validation.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please include why this is useful, and what behaviour change the user might experience.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

Copy link
Collaborator

@lornajane lornajane left a comment

Choose a reason for hiding this comment

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

Thank you for sorting this out and formatting the command pages! I think we should sort the parameters alphabetically rather than adding new ones at the end.

I commented on the changelog wording, and I think it should mention the additional parameter.

docs/commands/bundle.md Outdated Show resolved Hide resolved
docs/commands/preview-docs.md Outdated Show resolved Hide resolved
docs/commands/push.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@lornajane lornajane left a comment

Choose a reason for hiding this comment

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

One tiny nitpick to fix but otherwise, very happy to see this in !

---

Added the possibility to configure the linting severity level of the configuration file for all CLI commands.
Now, Redocly CLI will exit with an error if there are any issues with the configuration file, and the severity is set to `error`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, but without the "Now" (sorry, a small thing I know!)

@IgorKarpiuk IgorKarpiuk merged commit 0c52110 into main Oct 19, 2023
28 checks passed
@IgorKarpiuk IgorKarpiuk deleted the fix/config-severity branch October 19, 2023 13:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants