Add optional spell check to makefile to check for typos in tor source code.

changed milestone to %Tor: 0.3.4.x-final

added 034-triage-20180328 component::core tor/tor milestone::Tor: 0.3.4.x-final owner::alison points::0.5 priority::low reporter::fristonio resolution::implemented review-group-34 reviewer::asn severity::minor status::closed tor-comment type::enhancement labels

@teor misspell also provide a standalone binary, we can use it to check for the spelling mistakes. We can modify this script to fetch the binary from its git releases https://github.com/client9/misspell/blob/master/install-misspell.sh.

What do you think ?

Trac:
Username: fristonio

I'd rather have any script just "use misspell if installed". Downloading and running a binary on developer's machine is usually not what I'd expect a test script to do.

So what you are suggesting is that I should add an option to check for the typos errors, and whenever it is called it should checks if misspell is installed, if it is we should use to detect the mistakes and if not it will give the user instructions if they want to install it?

Trac:
Username: fristonio

Yes; that sounds fine.

Ok, I will start working on it then.

Trac:
Username: fristonio

I have created a patch for this ticket, here is a link to it

https://github.com/fristonio/tor-patches/blob/master/0001-Add-spell-check-to-makefile-to-check-for-typos-in-co.patch

I am also attaching the patch here just for convenience.

Trac:
Username: fristonio

0001-Add-spell-check-to-makefile-to-check-for-typos-in-co.patch

Misspell integraion with makefile to check for typos.

Trac:
Username: fristonio

Thanks for this patch. I missed it because this ticket was in the wrong component. (And using @teor doesn't work - you need to add "teor" to the CC field.)

Code review:

The makefile should check for misspell at runtime, not configure time. Otherwise, a user who reads your message and installs misspell will get very confused. Try the shell command called "command". Or look how it's done elsewhere in the makefile.

Also, markdown links don't work in most terminals. You are better to just link once at the end of the log message:

misspell(http://github.com/client9/misspell)

Trac:
Keywords: N/A deleted, tor-comment added
Component: Community to Core Tor/Tor
Milestone: N/A to Tor: 0.3.4.x-final
Points: N/A to 0.5
Status: new to needs_revision

Would you like to submit your patches as a tor branch? It would be easier for us.

To submit patches as a branch, use commands like these:

git checkout -b ticket25024 master
# make your changes here
git add -p
git commit
git push origin ticket25024

Thanks, I will add the check to the Makefile.am instead of configure.ac and will submit the patch as a branch as soon as I get the work done.

Trac:
Username: fristonio

I have made the changes but when I try to push the changes using git push origin ticket25024 I get 403 error.

Trac:
Username: fristonio

"origin" is the name of the remote you cloned from.

Maybe you need to add a remote called "github", that points to git@github.com:fristonio/tor-patches.git or https://github.com/fristonio/tor-patches.git

GitHub has some step by step instructions to help you: https://help.github.com/articles/adding-a-remote/

Oh, I thought that I need to push the branch to gitweb.torproject.org/tor.git

Here is a link to the branch for ticket fixup. https://github.com/fristonio/tor/tree/ticket25024

Trac:
Username: fristonio

The commit message is now incorrect: the makefile checks for misspell when "make check-typos" is called, not in configure.

Tor is spelt "Tor", not "TOR".

You're still using markdown links, which won't work in most terminals:

Install the latest version of misspell(https://github.com/client9/misspell#install)

Please use a link that is separated from other content using spaces.

Would you like to add "check-typos" to the "check" target?

To make changes to the commit message, you can either checkout a new branch, or amend this branch and force push.

Sorry that I messed up a bit there, I updated the changes you told https://github.com/fristonio/tor/tree/ticket25024

I also added check-typos to make check as it seems a test similar to check-spaces.

Trac:
Username: fristonio

Hi, sorry this hasn't been reviewed, it didn't have the right status for our review groups to pick it up. It should be reviewed in the next few weeks.

Trac:
Status: needs_revision to needs_review

Trac:
Keywords: N/A deleted, review-group-34 added

Reviewer week 03/19th

Trac:
Reviewer: N/A to asn

LGTM and works fine in my testing. I rebased it to master but there are still a few typos around the codebase, should we fix them as part of this ticket?

Also what's make check-local and is it used by anything? If it's used by something I dont want it to burp because of the warnings about misspell missing.

check-local is a dependency of check-am, which is a dependency of check.

Please see branch ticket25024 in my github repo (https://github.com/asn-d6/tor), which includes fristonio's patch, rebased to latest master, with some word changes to make it obvious that misspell is an optional feature.

I rebased to master so that it includes #23650 (moved) patch. However I noticed that there are still a few typos in the codebase. I started fixing them but then I noticed that some of them are not actually typos but are either spanish words, or partial-words because of the way C strings are formatted. e.g.:

  config_line_append(&labels, "Adios", "planetas");
./src/test/test_conscache.c:44:40: "planetas" is a misspelling of "planets"
  memcpy(ext_or_auth_cookie, "s beside you? When I count, ther", 32);
./src/test/test_extorport.c:246:58: "ther" is a misspelling of "there"

What should we do about these cases? We could use misspell's -i switch to ignore those corrections, but does this scale? We could disallow spanish words or partial words but that would be too authoritarian...

I'm gonna mark this as needs_revision until we find a solution to the above, since IMO it doesn't make sense to have a spellchecker that will always complain about non-issues.

Fristonio, what would you do here? :)

Trac:
Status: needs_review to needs_revision

I completely agree with your point here asn, this will be an issue if we add misspell to Tor. Also, the -i flag solution does not seem to be scalable at all. One possible solution I see here for this is to have some ignore rules feature in misspell something like what eslint have for JS wherein we can specify with the help of comments if we want to ignore a block of code from being checked. See here: [https://eslint.org/docs/user-guide/configuring#disabling-rules-with-inline-comments] What is your thought on this?

But as of now misspell does not have this feature. I will talk to the maintainer regarding this feature. Maybe I will send a PR myself for the same to the project if required. Till then we can keep the patch on hold.

Trac:
Username: fristonio

Replying to fristonio:

I completely agree with your point here asn, this will be an issue if we add misspell to Tor. Also, the -i flag solution does not seem to be scalable at all. One possible solution I see here for this is to have some ignore rules feature in misspell something like what eslint have for JS wherein we can specify with the help of comments if we want to ignore a block of code from being checked. See here: [https://eslint.org/docs/user-guide/configuring#disabling-rules-with-inline-comments] What is your thought on this?

I feel like that might be a bit too ugly for our codebase just for a spellcheck. Perhaps that's fine for tests tho. IMO all possible solutions are a bit ugly here: a) Inline comments to disable spellcheck b) Custom dictionary c) Custom ignore rules with -i.

I think we should decide how this feature should be used tho. If people are supposed to run this before every commit then it should have zero false positives. However, if that's just something that the maintainer runs before every stable release every few months, then perhaps some false positives are fine (or they can be removed with a custom dictionary).

In general, I don't have strong opinions here, and as long as this feature is optional as it is, perhaps we can just merge it as is. What do you think?

Trac:
Keywords: N/A deleted, 034-triage-20180328 added

Per our triage process, these tickets are pending removal from 0.3.4.

Trac:
Keywords: N/A deleted, 034-removed-20180328 added

I too don't have any strong opinion about this, as the above mentioned point seems quite valid. Maybe someone more experienced with Tor will have a better say here. :)

Trac:
Username: fristonio
Keywords: 034-removed-20180328 deleted, N/A added

OK. I think I'll merge_ready this. No reason to just hold this off until we find the perfect solution which obviously does not exist.

If Nick likes the idea too, let's get it merged, see how and if it gets used, see if the false-positives are problematic, and iterate on this with the various approaches from comment:22 . No reason to spend too much time rabitholing about this ticket.

Thanks for the feedback fristonio :)

Trac:
Status: needs_revision to merge_ready

I've added a few questions/comments to the branch. Please have a look and let me know if you think revisions are warranted?

Also, this needs a changes file.

Trac:
Status: merge_ready to needs_revision

FWIW, Nick's comments can be found at: https://github.com/asn-d6/tor/commit/aa6f498a07a3ef192bfe26420e49b16df47a003a#commitcomment-28491165

I have updated the changes here https://github.com/fristonio/tor/tree/ticket25024 Comment if more changes are required. :) Sorry for not being responsive for the last couple of days.

Trac:
Username: fristonio

Trac:
Status: needs_revision to needs_review

Fixes look like they are addressing Nick's comments.

I feel like + $(top_srcdir)/src/[^e]*/*.[ch] \ is kind of a inelegant way to exclude src/ext/ but I don't know a better way to do it apart from using find or something, and right now it should work OK. If this feature ends up being useful, and we ever add another directory in src/ that starts with e we can think of a better approach there.

Setting this to merge_ready so that Nick can inspect it again.

Trac:
Status: needs_review to merge_ready

Indeed, lgtm! Merging this to master. Thanks for the hard work, everybody!

Trac:
Resolution: N/A to implemented
Status: merge_ready to closed

asn, nickm thanks for your help and feedback.

Trac:
Username: fristonio

closed

changed time estimate to 4h

moved to tpo/core/tor#25024 (closed)

Add optional spell check to makefile to check for typos in tor source code.

Child items ...

Activity