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

Fix code examples syntax highlighting in YARD documentation #4

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ilyazub
Copy link
Contributor

@ilyazub ilyazub commented Dec 22, 2023

We with @Freaky updated the tests to make sure it pass and fixed the syntax highlighting for YARD.

Co-authored-by: Thomas Hurst <thomas@serpapi.com>
@ilyazub ilyazub requested a review from Freaky December 22, 2023 17:08
@ilyazub ilyazub self-assigned this Dec 22, 2023
Copy link

@Freaky Freaky left a comment

Choose a reason for hiding this comment

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

Thanks @ilyazub - a definite improvement. Hopefully we can push these new clients over the line in 2024.

@@ -420,12 +465,12 @@ The class SerpApi::Client (client side / ruby):
Et voila!

## Continuous integration
We love "true open source" and "continuous integration", and Test Drive Development (TDD).
We are using RSpec to test [our infrastructure around the clock]) using Github Action to achieve the best QoS (Quality Of Service).
We love "true open source" and "continuous integration", and Test Drive Development (TDD). We are using RSpec to test our infrastructure around the clock using Github Action to achieve the best QoS (Quality Of Service).
Copy link

Choose a reason for hiding this comment

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

This weird sentence needs more work I think. What's it even trying to say? What is "true" open source and why do it and CI need scare quotes? Is name-dropping more abbreviations better for SEO?

Suggested change
We love "true open source" and "continuous integration", and Test Drive Development (TDD). We are using RSpec to test our infrastructure around the clock using Github Action to achieve the best QoS (Quality Of Service).
We love [continuous integration](https://en.wikipedia.org/wiki/Continuous_integration) (CI) and [Test-Driven Development](https://en.wikipedia.org/wiki/Test-driven_development) (TDD) at SerpApi. We use RSpec and Github Actions to test our infrastructure around the clock, and that includes all changes to our clients.

Comment on lines +186 to +187
async_search = client.search search
async_search
Copy link

Choose a reason for hiding this comment

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

I suppose the intention is to help the user follow information flow by giving it a name? I think it just makes it look cluttered - experienced Ruby devs will think it's silly and inexperienced ones may be confused why there's a variable here that appears to do nothing.

Suggested change
async_search = client.search search
async_search
client.search search

Comment on lines +193 to +194
results = client.search_archive search[:id]
results
Copy link

Choose a reason for hiding this comment

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

Suggested change
results = client.search_archive search[:id]
results
client.search_archive search[:id]

Comment on lines +189 to +191
# Get an ETA using the last search scheduled time
bulk_search_eta = async_search_results.last[:search_metadata][:scheduled_at]
# After the searches are done processing (i.e., `bulk_search_eta`)
Copy link

Choose a reason for hiding this comment

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

We probably want to make it clearer there should be a sleep here.

Comment on lines +184 to +185
# Submit async searches
async_searches = searches.map do |search|
Copy link

@Freaky Freaky Dec 27, 2023

Choose a reason for hiding this comment

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

Probably want to avoid confusing the user about async vs scheduled by being more consistent in naming?

Suggested change
# Submit async searches
async_searches = searches.map do |search|
# Submit scheduled searches
scheduled_searches = searches.map do |search|

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.

2 participants