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

ci: spelling: update and include advice #5211

Merged
merged 1 commit into from
Apr 1, 2020

Conversation

jsoref
Copy link
Contributor

@jsoref jsoref commented Apr 1, 2020

Summary of the Pull Request

This updates the spell checker.
Scheduled checks should now function, this means that if someone has a fork and isn't allowing actions to run in their fork, this repository should be able to run a spell check against the PR similar to the way checks would be run otherwise.

References

This is designed to be merged after #5207

PR Checklist

  • Closes #xxx
  • CLA signed. If not, go over here and sign the CLA
  • Tests added/passed
  • Requires documentation to be updated
  • I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: ci: run spell check in CI, fix remaining issues #4799 (comment)

Detailed Description of the Pull Request / Additional comments

Note: due to a bug in a previous version, i'th was treated as an acceptable term. This update fixes that bug which makes the checking stricter. nth is in the dictionary, and thus I've used it instead.

You can see how advice works because I'm conveniently building on top of an unhappy commit. If @DHowett-MSFT / @miniksa / @zadjii-msft have input about how the advice should be worded, we can certainly adjust that here (or you're welcome to tune it later).

Validation Steps Performed

I pushed this commit (and a couple of variants) to my fork. That resulted in jsoref@f2d57bc (the ❌).

Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@DHowett-MSFT DHowett-MSFT left a comment

Choose a reason for hiding this comment

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

This is really great. Thank you.

@DHowett-MSFT DHowett-MSFT merged commit 713550d into microsoft:master Apr 1, 2020
@jsoref jsoref deleted the spell-check branch April 1, 2020 19:15
@jsoref
Copy link
Contributor Author

jsoref commented Apr 1, 2020

@j4james would #5211 (comment) have helped?

My goal is to make the feedback as helpful/understandable as possible. If the problem is that the disclosure widget hid too much, I can work on that too (in the interim, the advice text could suggest expanding it).

@j4james
Copy link
Collaborator

j4james commented Apr 1, 2020

@j4james would #5211 (comment) have helped?

Yeah, that definitely would have helped. The only thing I might suggest is clarifying that the first point is for names of people. I was a little confused about that, until I read the README in the dictionary directory.

@jsoref
Copy link
Contributor Author

jsoref commented Apr 1, 2020

I basically created a separate thing because it's reasonably distinct from other things.

The microsoft.txt file in my imagination is things like codenames/software/projects that the code might be built on top of. Whereas names is people / external entities.

I also included Firefox in names.txt. It's really up to the team how they want to use the files. Admittedly, the readme explicitly says people. So,...

Someone else can reorganize them if they can think of better organizing criteria. I'm not attached to the scheme, it's just there as a sample so that the project can experiment/grow into using it in the way that best fits their needs.

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