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 Spelling #814

Open
wants to merge 14 commits into
base: gh-pages
Choose a base branch
from
Open

Fix Spelling #814

wants to merge 14 commits into from

Conversation

dblythy
Copy link
Member

@dblythy dblythy commented Feb 2, 2021

I've worked through the docs and spellchecked where I can.

Let me know your thoughts!

_includes/common/security.md Outdated Show resolved Hide resolved
_includes/android/files.md Outdated Show resolved Hide resolved
CODE_OF_CONDUCT.md Show resolved Hide resolved
@TomWFox
Copy link
Contributor

TomWFox commented Feb 2, 2021

I would really like to get a spell check running on every PR as it’s time consuming to correct & ensure good spelling.

@dblythy
Copy link
Member Author

dblythy commented Feb 2, 2021

I would be happy to set that up if you'd like? I'll include the whitelist I started building.

@TomWFox
Copy link
Contributor

TomWFox commented Feb 2, 2021

Absolutely! I was thinking it would be best to use GH actions unless you have other ideas...

@dblythy
Copy link
Member Author

dblythy commented Feb 2, 2021

My thoughts exactly. I'll add that to this PR.

@dblythy
Copy link
Member Author

dblythy commented Feb 2, 2021

I had to fork the markdown spell check and make a few changes otherwise the whitelist (.spelling) would be literally 1000s of lines long.

Let me know what you think of this implementation. Please have a look if I accidentally added any non-words to the whitelist by mistake.

@dblythy
Copy link
Member Author

dblythy commented Feb 2, 2021

Not sure why it's not showing here but the CI runs through all MD files and checks spelling.

Here's the result

Might need a coding genius such as @mtrezza to look over just to make sure it's ok to use this.

npm run spellcheck will spellcheck the files with interactive UI to replace the text.

npm run spelltest is used by the CI as we're not interested as how interactive it is.

@TomWFox
Copy link
Contributor

TomWFox commented Feb 5, 2021

This looks good - and nice that you can easily run it locally! It might be nice to have it add a comment with any issues at some point but probably more effort than it's worth for now.

I forked the repo into the org and merged your changes - could you point to the org's fork?

@TomWFox
Copy link
Contributor

TomWFox commented Feb 5, 2021

A couple question come to mind...

  1. Does it spell check code? - inline code and/or code blocks
  2. Does it spell check code comments?

@dblythy
Copy link
Member Author

dblythy commented Feb 5, 2021

It previously didn't, but I've submitted a PR to make sure it does

@TomWFox
Copy link
Contributor

TomWFox commented Feb 13, 2021

It actually makes me unreasonably happy that it removes extra spaces, I've spend too much time removing double spaces from PRs!

@@ -734,7 +734,7 @@ That would customize the string to "In progress". The key on the left is the ori

Say, you would like to customize the error message in `PFSignUpViewController` that says "The email address "andrew@x" is invalid. Please enter a valid email." You are not sure how to enter this into `Localizable.strings` because it contains a variable.

Included in the Parse SDK is a file named `Localizable.string` which includes all the localizable keys in the Parse framework. Browsing this file, developers can find the key for the string they would like to customize. You notice that the string `"The email address \"%@\" is invalid. Please enter a valid email."` is a key in the file. In your own `Localizable.strings`, you can then enter:
Included in the Parse SDK is a filenamed `Localizable.string` which includes all the localizable keys in the Parse framework. Browsing this file, developers can find the key for the string they would like to customize. You notice that the string `"The email address \"%@\" is invalid. Please enter a valid email."` is a key in the file. In your own `Localizable.strings`, you can then enter:
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be file named

@TomWFox
Copy link
Contributor

TomWFox commented Feb 13, 2021

Thanks for extending it code, I'm not actually sure if it's a good idea though.

I can see an issue on line 351 of unity/objects - MonoBehaviour was changed to MonoBehavior; from here it appears that the former is correct.

When PRs introduce new code it may mean adding more and more to the .spelling list which increased from ~200 lines to ~900 due to this change.

If we can, perhaps only spell checking code comments and strings would be a happy medium? - although maybe even that adds too much to the .spelling list?

@mtrezza
Copy link
Member

mtrezza commented Sep 10, 2021

@dblythy Do you know why this PR has become stale? The docs have been somewhat neglected in the past, but we'll pick this up now and close the open PRs and issues. Do you think it makes sense to merge this before we start?

@dblythy
Copy link
Member Author

dblythy commented Sep 12, 2021

Should be good for review @mtrezza

1.0a
unlinking
nuget-link
5.2.x
Copy link
Member

Choose a reason for hiding this comment

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

I assume this file is the "ignore" list for the spell check. There are quite a lot of version numbers, does this accept a regex to cover all these version numbers?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't believe so

Copy link
Member

@mtrezza mtrezza Sep 22, 2021

Choose a reason for hiding this comment

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

Could you sort the contents of this file alphabetically? Just for better overview. The file looks suspiciously long, maybe there al alphabetical order can give us a better idea about why it contains so many entries.

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.

3 participants