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

Add a notice about overtesting non-standard syntaxes #6572

Closed

Conversation

ybiquitous
Copy link
Member

@ybiquitous ybiquitous commented Jan 10, 2023

Which issue, if any, is this issue related to?

See the discussion on PR #6556.

Is there anything in the PR that needs further explanation?

This PR is just a document fix.

My English wording may be a little unnatural. Any advice is welcome.

@changeset-bot
Copy link

changeset-bot bot commented Jan 10, 2023

⚠️ No Changeset found

Latest commit: 26a546e

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@ybiquitous ybiquitous marked this pull request as ready for review January 10, 2023 14:15
@ollie-iterators
Copy link

ollie-iterators commented Jan 10, 2023

Some advice on the wording:

  1. Move the second paragraph above the first.
  2. "Therefore, a rule can delegate tests for such syntaxes to those utilities in most cases." could be changed to "Therefore, tests for those syntaxes could be delegated to those utilities in most cases"
  3. "leads to unhealthy in our project and will be difficult to maintain" could be changed to : "makes it harder to maintain the project"
  4. "Please take care of increasing unnecessary tests" could be changed to "Please do not add unnecessary tests"

@jeddy3
Copy link
Member

jeddy3 commented Jan 10, 2023

Let's hold off merging this until #6573 lands as it'll be easier to manage the conflict here than there.

@ybiquitous
Copy link
Member Author

@ollie-iterators Thanks for the advice. I've changed it via 26a546e.

@jeddy3 Thanks for writing the docs. If you don't mind, could you merge this PR change into PR #6573 and close this PR? (I'm not stuck on the current wording so that you could arrange it)

@jeddy3
Copy link
Member

jeddy3 commented Jan 12, 2023

If you don't mind, could you merge this PR change into PR #6573 and close this PR? (I'm not stuck on the current wording so that you could arrange it)

Sure thing. Done in d7ef445

@jeddy3 jeddy3 closed this Jan 12, 2023
@jeddy3 jeddy3 deleted the docs/add-notice-for-overtesting-non-standard-syntaxes branch January 12, 2023 10:56
@ybiquitous
Copy link
Member Author

Thank you!

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

Successfully merging this pull request may close these issues.

None yet

4 participants