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

Spelling #529

Merged
merged 6 commits into from
Apr 19, 2021
Merged

Spelling #529

merged 6 commits into from
Apr 19, 2021

Conversation

jsoref
Copy link
Contributor

@jsoref jsoref commented Apr 18, 2021

This PR corrects misspellings identified by the check-spelling action.

The misspellings have been reported at jsoref@5960a49#commitcomment-49670375

The action reports that the changes in this PR would make it happy: jsoref@3d59a7c

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>
Copy link
Member

@dscho dscho left a comment

Choose a reason for hiding this comment

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

The fixture should probably not be changed, as it is a verbatim copy of a mail that has been sent to the Git mailing list.

If it helps, though, by shutting up some spell-correction check, I'm okay with applying this as-is.

I'll leave @webstech some time to chime in.

@@ -405,7 +405,7 @@ Release tarballs are available at:
Will merge to 'next' after waiting for a few more days for comments.


* cw/pack-vs-bigfilethreashold (2021-02-09) 2 commits
* cw/pack-vs-bigfilethreshold (2021-02-09) 2 commits
Copy link
Member

Choose a reason for hiding this comment

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

Technically, this is incorrect, as I copied this email verbatim. Changing it would no longer be verbatim.

In practice, it probably does not matter much, so I'm fine with this change anyway.

@@ -683,7 +683,7 @@ Release tarballs are available at:
- unix-socket: do not call die in unix_stream_connect()
- unix-socket: add no-chdir option to unix_stream_listen()
- unix-socket: add options to unix_stream_listen()
- unix-socket: elimiate static unix_stream_socket() helper function
- unix-socket: eliminate static unix_stream_socket() helper function
Copy link
Member

Choose a reason for hiding this comment

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

Same here.

@@ -756,7 +756,7 @@ Release tarballs are available at:

The common code to deal with "chunked file format" that is shared
by the multi-pack-index and commit-graph files have been factored
out, to help codepaths for both filetypes to become more rebust.
out, to help codepaths for both filetypes to become more robust.

Will merge to 'next' after waiting for a few days.
Copy link
Member

Choose a reason for hiding this comment

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

And here.

@@ -1148,7 +1148,7 @@ Release tarballs are available at:
. update-index: use istate->cache_nr over active_nr
. update-index: use istate->cache over active_cache
. update-index: drop the_index, the_repository
. rm: remove compatilibity macros
. rm: remove compatibility macros
. mv: remove index compatibility macros
(this branch uses ag/merge-strategies-in-c.)

Copy link
Member

Choose a reason for hiding this comment

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

And here.

@jsoref
Copy link
Contributor Author

jsoref commented Apr 18, 2021


The tool would be happy to ignore files matching \.mbox$ if that's preferable. It doesn't matter to me.

I start my contributions with small things to learn how to make contributions in general. (I have thoughts about the actual content of this repository, but if I can't figure out how to make a tiny contribution here, it wouldn't make sense for me to try to send something more significant.)

I do not understand how this repository works. Nor do I understand how contributing to the git repository works -- gitgitgadget/git#934 (comment).

So I'm not going to do anything in response to either until someone says something akin to "please make this change".

@webstech
Copy link
Contributor

@dscho I do not have an issue with the changes to the 'verbatim' email copy. It could be changed in the future to add additional tests.

@jsoref I see that you are associated with the check-spelling action. You mentioned the git repo in your last comment. Do you think these repos needs a spell check? There are only two spelling errors (guilty of one) in this repo. Are you going to contribute corrections or want to introduce check-spelling into the workflow,?

@jsoref
Copy link
Contributor Author

jsoref commented Apr 19, 2021

@webstech: I generally don't offer the workflow unsolicited. I'm happy to contribute it. I think most repositories benefit from having a spell checker. Two typos is definitely on the low side, but typically the count grows as a project grows (projects can easily exceed 100 or 1000 with size and age).

@dscho
Copy link
Member

dscho commented Apr 19, 2021

I generally don't offer the workflow unsolicited. I'm happy to contribute it

That would be great!

Is it possible to somehow combine the configuration with cSpell's? We have a ton of cSpell configuration:

"cSpell.words": [
"Arnfjörð",
"Bjarmason",
"Cygwin",
"Git's",
"Hamano",
"ISMTP",
"Junio",
"Nguy",
"Nguyễn",
"Ngọc",
"Pratyush",
"Probot",
"Schindelin",
"Thái",
"Truthy",
"VSTS",
"Yadav",
"addressparser",
"amlog",
"autocrlf",
"avarab",
"basedon",
"busybox",
"coverageprocessor",
"dstolee",
"dugite",
"eqeqeq",
"fooba",
"gitgitgadget",
"gitster",
"gmail",
"gmane",
"googlegroups",
"imaps",
"insn",
"isnan",
"issuecomment",
"latesttag",
"latesttags",
"libqp",
"maildir",
"mailsplit",
"maint",
"mbox",
"mergeable",
"octo",
"octokit",
"onelines",
"pclouds",
"pobox",
"promisify",
"publishtoremote",
"rebased",
"refname",
"repo",
"smee",
"smtp",
"stolee",
"stripspace",
"superset",
"taggerdate",
"tbdiff",
"unportable",
"vger",
"webhook",
"webhooks",
"worktree",
"yadavpratyush",
"Ævar"
],
"cSpell.ignoreRegExpList": [
"CIimapHost",
"CIsmtphost",
"CIsmtppass",
"CIsmtpopts",
"users\\.noreply\\.github\\.com",
],
"cSpell.language": "en-US",
"cSpell.enabledLanguageIds": [
"git-commit",
"html",
"javascript",
"json",
"markdown",
"plaintext",
"typescript"
],
"cSpell.ignorePaths": [
"node_modules",
"**/node_modules"
]

@dscho dscho merged commit e922fb6 into gitgitgadget:main Apr 19, 2021
@jsoref jsoref deleted the spelling branch April 19, 2021 10:48
@jsoref
Copy link
Contributor Author

jsoref commented Apr 19, 2021

@webstech, @dscho: this is the current commit for the action: jsoref@spell-check

I'm hoping to release 0.0.18-alpha this week. I just finished off one added feature for it. I need to do a bit more testing (including end user testing), update some graphics (usability thing), and write release notes.

I think the most interesting thing I've seen in cSpell is that it has
Area dictionaries, which has been on my to-do list for a while. I'll probably try to just add direct support for them for the release after the pending one -- it shouldn't be much work since I already have a default code path to retrieve the main dictionary.

Would you want an "Issue" for this to track the task, or should it just sit until I'm ready?

@dscho
Copy link
Member

dscho commented Apr 19, 2021

this is the current commit for the action: jsoref@spell-c

Great!

One thing, though: rather than committing the documentation for the spell checker (e.g. README.md), maybe we can have a link in the commit message instead?

Speaking of commit messages: this is a great place to fill in the interested reader about background (such as: where it was suggested to use it, what PR was necessary because the check was not yet added, etc).

the most interesting thing I've seen in cSpell is that it has
Area dictionaries, which has been on my to-do list for a while.

I was mostly interested in learning whether there is a way we can provide those patterns that works both for cSpell and that GitHub Action, without having to duplicate the lists.

Would you want an "Issue" for this to track the task, or should it just sit until I'm ready?

How about a PR? You can mark it as "Draft" to indicate that you're not done with it yet.

@jsoref
Copy link
Contributor Author

jsoref commented Apr 19, 2021

To the other bit about cSpell, I haven't spent much time thinking about the best way to interoperate with them. Definitely being able to borrow their dictionaries would be nice. Supporting their entire configuration languages might be a bit much. Generally there's a pretty straightforward mapping between spell checkers since they all more or less have to do the same things:

  • define a dictionary (+supplements) (-removals) -- these can be proprietary binary formats or plain text (I use single word per line, cSpell seems to use single term per line -- where a term can include spaces)
  • define files to check (-exclusions) -- I use regular expressions, some projects use globs (some use gitignore)
  • define patterns (typically regular expressions) to ignore content
  • list of non dictionary words that aren't problematic
  • help users interact w/ the tool

And possibly handle multiple programming languages differently. -- My current plan for this is that the spell-checker would use GHA matrix for the different languages. But I haven't run into a case where it's mattered enough (as opposed to just merging multiple dictionaries). I plan to experiment with this more for llvm (whose running time is currently way too long).

@dscho
Copy link
Member

dscho commented Apr 19, 2021

Supporting their entire configuration languages might be a bit much.

Right. I am too unfamiliar with cSpell to know the answer off the top of my head to the question whether it is possible to provide those pattern lists as plain .txt files. That would definitely make it most convenient to share the same lists between different spell checkers.

@jsoref jsoref mentioned this pull request Apr 19, 2021
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.

None yet

3 participants