8000 feat: remove --lint, --preprocess, and --decorate options from the join command by DmitryAnansky · Pull Request #1507 · Redocly/redocly-cli · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

feat: remove --lint, --preprocess, and --decorate options from the join command #1507

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 9 commits into from
Apr 4, 2024
Merged

feat: remove --lint, --preprocess, and --decorate options from the join command #1507

merged 9 commits into from
Apr 4, 2024

Conversation

DmitryAnansky
Copy link
Contributor
@DmitryAnansky DmitryAnansky commented Mar 26, 2024 •
edited
Loading

What/Why/How?

Remove options from the join command:

  • lint
  • decorate
  • preprocess

Reference

Closes #1397

Testing

Screenshots (optional)

Check yourself

  • Code changed? - Tested with redoc/reference-docs/workflows (internal)
  • All new/updated code is covered with tests
  • New package installed? - Tested in different environments (browser/node)

Security

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

@changeset-bot changeset-bot
Copy link
changeset-bot bot commented Mar 26, 2024 •
edited
Loading

🦋 Changeset detected

Latest commit: 11e7ffc

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 GitHub Actions
Copy link
Contributor
github-actions bot commented Mar 26, 2024 •
edited
Loading
Command Mean [s] Min [s] Max [s] Relative
redocly lint packages/core/src/benchmark/benches/rebilly.yaml 1.019 ± 0.045 0.971 1.139 1.02 ± 0.05
redocly-next lint packages/core/src/benchmark/benches/rebilly.yaml 0.998 ± 0.012 0.981 1.024 1.00

@github-actions GitHub Actions
Copy link
Contributor
github-actions bot commented Mar 26, 2024 •
edited
Loading

Coverage report

St.❔
 Category Percentage Covered / Total
🟡 Statements 76.84% 4415/5746
🟡 Branches 67.06% 2396/3573
🟡 Functions 70.58% 734/1040
🟡 Lines 77% 4145/5383
Show files with reduced coverage 🔻
St.❔
 File Statements Branches Functions Lines
🟡
... / join.ts
68.87% (+1.29% 🔼)
 54.7%
66.67% (+1.36% 🔼)
70.39% (+1.41% 🔼)

Test suite run success

721 tests passing in 101 suites.

Report generated by 🧪jest coverage report action from 11e7ffc

@DmitryAnansky DmitryAnansky force-pushed the feat/remove-combination-operations-from-join branch from f2f3b6e to 9bd6abf Compare March 27, 2024 11:12
@DmitryAnansky DmitryAnansky marked this pull request as ready for review March 27, 2024 11:25
@DmitryAnansky DmitryAnansky requested review from a team as code owners March 27, 2024 11:25
lornajane
lornajane approved these changes Mar 27, 2024
View reviewed changes
Copy link
Contributor
@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.

Looks good and locally seems to behave as I would expect. Thanks!

@DmitryAnansky DmitryAnansky requested a review from HCloward March 27, 2024 13:58
@tatomyr
Copy link
Collaborator
tatomyr commented Apr 1, 2024

I don't reckon this PR completely resolves the original issue.
@lornajane, besides the command line options, decorators and preprocessors can be configured through a config file (i.e. in the apis section). They will still be executed. Did you mean only to remove the options or the entire possibility to fire decorators/preprocessors with the join command?

@lornajane
Copy link
Contributor

Ah, yes. The join command should not do any preprocessing at all, for any reason. That functionality should be only in bundle.

@DmitryAnansky
Copy link
Contributor Author

Ah, yes. The join command should not do any preprocessing at all, for any reason. That functionality should be only in bundle.

So in current changes we are skipping default one


 const preprocessors = new Set([
    ...Object.keys(config.styleguide.preprocessors.oas3_0),
    ...Object.keys(config.styleguide.preprocessors.oas3_1),
    ...Object.keys(config.styleguide.preprocessors.oas2),
  ]);
  config.styleguide.skipPreprocessors(Array.from(preprocessors));

@tatomyr
Copy link
Collaborator
tatomyr commented Apr 2, 2024 •
edited
Loading

Hmm. So, the decorators/preprocessors are getting explicitly suppressed in the code? That makes me think. Since join does bundle every API document under the hood before the actual joining, wouldn't it be expected to decorate those documents the same way the bundle command does?

One more thing. Redocly CLI actually runs preprocessors not only on the bundle command but also when linting. Should we also remove the ability to preprocess there? I'm not certain, as it could impact the VS Code extension user experience.

Anyway, if we are removing those options, I'd expect to have a document explaining how to switch from the current usage model to the new one. We have such one for the --lint option, but --decorate and --preprocess are different a bit. Could we put a link to a guide or something?

cc @lornajane

@lornajane
Copy link
Contributor

My perspective is more about how the user sees the feature. It makes sense that join would bundle behind the scenes - but does the user know or care about that? I didn't at all expect that a join command would also perform our transform operations, it's fine to bundle and resolve the refs, but I don't think it's useful for decorators to be applied here. Quite possibly people will need to make some decorator changes as well, either before or after joining, but we have that functionality and they can built it into their pipelines in a way that's clear.

@lornajane
Copy link
Contributor

Updated to add: let's signpost the decorators from the join command docs.

@DmitryAnansky
Copy link
Contributor Author
DmitryAnansky commented Apr 3, 2024 •
edited
Loading

signpost
@lornajane
Could you please give me more details, didn't get signpost part.
Will it be enough to update docs, or we need extra hints in console ?

@DmitryAnansky DmitryAnansky force-pushed the feat/remove-combination-operations-from-join branch from 01bb3ea to a83a744 Compare April 3, 2024 13:55
tatomyr
tatomyr approved these changes Apr 3, 2024
View reviewed changes
Copy link
Collaborator
@tatomyr tatomyr left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks!

@lornajane
Copy link
Contributor

"Signpost" literally a post with a sign on it!! But in documentation terms, it means to help users to find the direction, by adding information and a link about something to "signpost" them to another thing. In this instance, you can translate my request as "let's add some information and a link from the join documentation to the decorators documentation"

@tatomyr tatomyr changed the title feat: remove preprocess option from join command feat: remove --lint, --preprocess, and --decorate options from the join command Apr 4, 2024
@DmitryAnansky DmitryAnansky force-pushed the feat/remove-combination-operations-from-join branch from 1b47dcf to 845c708 Compare April 4, 2024 10:01
@DmitryAnansky
Copy link
Contributor Author

"Signpost" literally a post with a sign on it!! But in documentation terms, it means to help users to find 8000 the direction, by adding information and a link about something to "signpost" them to another thing. In this instance, you can translate my request as "let's add some information and a link from the join documentation to the decorators documentation"

Added similar console notification messages as for bundle deprecated options.
@lornajane , please take a look.

lornajane
lornajane approved these changes Apr 4, 2024
View reviewed changes
Copy link
Contributor
@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.

Pushed a docs change, hope you don't mind. It looks good.

@DmitryAnansky DmitryAnansky merged commit b0b50da into main Apr 4, 2024
31 checks passed
@DmitryAnansky DmitryAnansky deleted the feat/remove-combination-operations-from-join branch April 4, 2024 12:50
@tatomyr tatomyr mentioned this pull request Feb 7, 2025
Open
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lornajane lornajane lornajane approved these changes

@tatomyr tatomyr tatomyr approved these changes

@HCloward HCloward Awaiting requested review from HCloward

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove combination operations from join
4 participants
@DmitryAnansky @tatomyr @lornajane @HCloward
0