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

Correct typos and language #457

Closed

Conversation

DannyvdSluijs
Copy link
Contributor

This PR addresses several typos and language constructs found throughout the sources. I've groups the fixes in individual commits in an attempt to make review easiest. Making individual PR for each type of typo feels weird and one for each file feels like an overhead.

Description

This PR address typos as found bij PHPStorm using the Problems view. Especially for the one where the introduction of a comma was suggested I've been withholding in addressing each issues as found by the editor. Especially for short sentences this would be not that helpfull.

Suggested changelog entry

Correct typos and language

Related issues/external references

N/A

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
    • This change is only breaking for integrators, not for external standards or end-users.
  • Documentation improvement

PR checklist

  • I have checked there is no other PR open for the same change.
  • I have read the Contribution Guidelines.
  • I grant the project the right to include and distribute the code under the BSD-3-Clause license (and I have the right to grant these rights).
  • I have added tests to cover my changes.
  • I have verified that the code complies with the projects coding standards.
  • [Required for new sniffs] I have added XML documentation for the sniff.

@DannyvdSluijs
Copy link
Contributor Author

I've noticed some test are failing which I'll address later tonight or at least this week.

@jrfnl
Copy link
Member

jrfnl commented Apr 22, 2024

@DannyvdSluijs Thanks for your willingness to contribute, but even just a quick glance at the changes, shows me quite a few are incorrect/introducing grammatical errors instead of fixing them.

Please do a very very very careful review of your proposed changes and update the branch to remove all the incorrect ones.

I'm tempted to close this PR as it is now, with an option to re-open it once you've had a chance to work out the kinks.

You may also want to consider splitting the PR in one which only touches docs files only, like the changelog and the readme and one which touches code files to make it more manageable.

@DannyvdSluijs
Copy link
Contributor Author

Seeing the amount of files changes here in GH I can see this should be split.

but even just a quick glance at the changes, shows me quite a few are incorrect/introducing grammatical errors

I’m curious which ones as the detection was done by PHPStorm but every change was intentional and by hand. If you can share them that would be great.

I’ll close this for now and keep my branch as a reference and work on smaller PR’s addressing one type of issue at a time.

@jrfnl
Copy link
Member

jrfnl commented Apr 23, 2024

@DannyvdSluijs Thank you for understanding. I'll leave some comments inline to point out things I noticed when I first had a quick look.

Parse/compile errors can take various forms. Some can be handled without much problems by PHPCS, some can not.
Parse/compile errors can take various forms. Some can be handled without many problems by PHPCS, some can not.
Copy link
Member

Choose a reason for hiding this comment

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

"without much problems" is an expression and is correct. "without many problems" changes the meaning of the sentence.

@@ -0,0 +1,3 @@
<?php
Copy link
Member

Choose a reason for hiding this comment

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

This file doesn't belong here.

@@ -61,7 +61,7 @@
* @property bool $stdin Read content from STDIN instead of supplied files.
* @property string $stdinContent Content passed directly to PHPCS on STDIN.
* @property string $stdinPath The path to use for content passed on STDIN.
* @property bool $trackTime Whether or not to track sniff run time.
Copy link
Member

@jrfnl jrfnl Apr 23, 2024

Choose a reason for hiding this comment

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

There is nothing wrong with the original text as far as I can see. (here and below)

// Special case for Widgets cause they are, well, special.
// Special case for Widgets because they are, well, special.
Copy link
Member

Choose a reason for hiding this comment

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

I'd accept this change, but cause is valid English, though maybe colloquial.

// then the opener is sharing its closer with other tokens. We only
// then the opener is sharing it's closer with other tokens. We only
Copy link
Member

Choose a reason for hiding this comment

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

its is correct.

@@ -17,7 +17,7 @@ declare(encoding='utf-8');
* that is available through the world-wide-web at the following URI:
* http://www.php.net/license/3_0.txt. If you did not receive a copy of
* the PHP License and are unable to obtain it through the web, please
* send a note to license@php.net so we can mail you a copy immediately.
* send a note to license@php.net, so we can mail you a copy immediately.
Copy link
Member

Choose a reason for hiding this comment

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

This is a predefined text in the tests and should not be changed.

// We found the a token that looks like the opener, but it's nested differently.
// We found the token that looks like the opener, but it's nested differently.
Copy link
Member

Choose a reason for hiding this comment

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

The "the" should be removed, the "a" is correct.

// This opener shares its closer with the previous opener,
// This opener shares it's closer with the previous opener,
Copy link
Member

Choose a reason for hiding this comment

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

its is correct.

// If it has an XML extension, let's at least try it.
// If it has an XML extension, lets at least try it.
Copy link
Member

Choose a reason for hiding this comment

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

I believe the let's is correct here, but wouldn't mind a second opinion.

Copy link
Member

Choose a reason for hiding this comment

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

"let's" is short for "let us", which is correct in this context. "lets" is a conjugation of "to let" - eg, "I let", "you let", "she lets".

* with the exception of select tests for the Config class itself.
* except select tests for the Config class itself.
Copy link
Member

Choose a reason for hiding this comment

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

Nothing wrong with the original phrasing.

* Verify recognition of PHP8 constructor with both property promotion as well as normal parameters.
* Verify recognition of PHP8 constructor with both property promotion and normal parameters.
Copy link
Member

Choose a reason for hiding this comment

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

This changes the meaning (though subtly).

* @param int|string $startTokenType The type of token(s) to look for for the start of the string.
* @param int|string $startTokenType The type of token(s) to look for the start of the string.
Copy link
Member

Choose a reason for hiding this comment

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

This change is incorrect. The original may look strange with the double for, but is correct. (same below)

* Test the tokens in the type of a typed constant as well as the constant name are tokenized correctly.
* Test the tokens in the type of typed constant as well as the constant name are tokenized correctly.
Copy link
Member

Choose a reason for hiding this comment

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

This change makes the sentence grammatically incorrect.

@jrfnl
Copy link
Member

jrfnl commented Apr 23, 2024

@DannyvdSluijs I haven't gone through all of it, but just left some pointers now. Hope they help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants