Skip to content

fix(specs): New Crawler API parameter - ignorePaginationAttributes #4614

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

Merged
merged 10 commits into from
Mar 20, 2025

Conversation

gazconroy
Copy link
Contributor

🧭 What and Why

New configuration parameter added to the Crawler API.

Changes included:

  • Added the ignorePaginationAttributes parameter
  • Extended text descriptions for some parameters and actions
  • Added some examples
  • Changed some number types to integer types
  • Enforced double-spacing for paragraphs.

@gazconroy gazconroy requested a review from a team as a code owner March 18, 2025 12:43
@gazconroy gazconroy requested review from Fluf22 and millotp March 18, 2025 12:43
@algolia-bot
Copy link
Collaborator

algolia-bot commented Mar 18, 2025

✔️ Code generated!

Name Link
🪓 Triggered by 633905d32c6dd97db19d3afff6116c3e77eb4cd7
🍃 Generated commit 103b21a8ced68e74fd413416ef2ebae2d5dc64fa
🌲 Generated branch generated/fix/crawler-pagination
📊 Benchmark results

Benchmarks performed on the method using a mock server, the results might not reflect the real-world performance.

Language Req/s
javascript 1635
go 1587
php 1527
csharp 1347
java 1118
python 1086
ruby 836
swift 694
scala 23

@gazconroy gazconroy requested a review from kai687 March 18, 2025 12:44
shortcuts
shortcuts previously approved these changes Mar 18, 2025
Copy link
Member

@shortcuts shortcuts 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! just one question


It doesn't replace:


Copy link
Member

Choose a reason for hiding this comment

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

are all of those spaces expected?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Double spacing makes sure paragraphs render properly and spacing between list elements stops them running into each other

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it's necessary for specs in this repo as you can see with the other API specs. The sources don't have this formatting, but in the generated, complete specs, you'll find the double spacing. Maybe something to do with YAML parsing/writing.

Copy link
Contributor

Choose a reason for hiding this comment

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

In the source, you'll notice that long description fields are declared as literal block scalars with the | symbol, which means keep newlines as indicated.

During the bundling of the spec, and I don't know why, most of these are converted into folded block scalars with the > symbol, which means newlines are replaced by a space, two newlines are replaced by one newline.

I find the | style more readable as it leads to more compact blocks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll keep an eye on this when the bundle is built. I did notice some discrepancies in spacing that I manually corrected for the Mintlify prototype.

@gazconroy gazconroy marked this pull request as draft March 18, 2025 16:13
@gazconroy
Copy link
Contributor Author

Popping this on hold for now. I've just spotted some duplication in my updates...

@gazconroy gazconroy marked this pull request as ready for review March 19, 2025 12:06
@gazconroy gazconroy requested a review from shortcuts March 19, 2025 12:06
Copy link
Member

@shortcuts shortcuts left a comment

Choose a reason for hiding this comment

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

flawless

@shortcuts shortcuts enabled auto-merge (squash) March 20, 2025 08:33
@shortcuts shortcuts merged commit d231d08 into main Mar 20, 2025
28 checks passed
@shortcuts shortcuts deleted the fix/crawler-pagination branch March 20, 2025 08:47
algolia-bot added a commit that referenced this pull request Mar 20, 2025
…4614) (generated) [skip ci]

Co-authored-by: gazconroy <[email protected]>
Co-authored-by: Gary Conroy <[email protected]>
Co-authored-by: Kai Welke <[email protected]>
Co-authored-by: Clément Vannicatte <[email protected]>
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.

4 participants