-
-
Notifications
You must be signed in to change notification settings - Fork 12
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 typos in documentation #45
Conversation
@@ -104,7 +104,7 @@ This means that you control how many operations can be executed concurrently. | |||
If you add a job to the queue and it still below the limit, it will be executed | |||
immediately. If you keep adding new jobs to the queue and its concurrency limit | |||
is reached, it will not start a new operation and instead queue this for future | |||
execution. Once one of the pending operations complete, it will pick the next | |||
execution. Once one of the pending operations is complete, it will pick the next |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that the exact same sentence can also be found in the Queue
class, I would like to make sure this is consistent. In either case I'm not sure about this one, feels like both would be valid variants, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would agree to this. Didn't saw it in the Queue
class. I was fast eyeballing the sentences, so next time I will take more time in future PR's. Thank you. 👍
README.md
Outdated
@@ -183,7 +183,7 @@ $q = new Queue(10, null, function ($url) use ($browser) { | |||
$promise = $q($url); | |||
``` | |||
|
|||
The `$q` instance is invokable, so that invoking `$q(...$args)` will | |||
The `$q` instance is invocable, so that invoking `$q(...$args)` will |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about this one, feels like both would be valid variants, no? Couldn't find a definitive source, but perhaps you have more insights? It looks like "invokable" is used more often than "invocable" in a programming context?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was researching the topic and it seems like you are definitely right with that one. It was also said in #44 as I saw. Thank you for this information. 🌟
src/Queue.php
Outdated
@@ -338,7 +338,7 @@ public function __construct($concurrency, $limit, $handler) | |||
} | |||
|
|||
/** | |||
* The Queue instance is invokable, so that invoking `$q(...$args)` will | |||
* The Queue instance is invocable, so that invoking `$q(...$args)` will |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(See above, same comment applies here)
Thanks for your help @clue. I guess now should everything be okay. I was changing "invocable" back to "invokable" and I also changed the sentence in the |
In my opinion, since the sentence can be interpreted in two ways, I don't think any changes are necessary for the documentation part. To maintain the developer's linguistic style, I would close the ticket and and keep the newly acquired information about 'invokable' in mind for future typo fixes. |
This pull request fixes a few typos with the help of https://github.com/szepeviktor/typos-on-you.
Builds on top of clue/reactphp-redis#146 and #44.