Opened 8 months ago

Closed 5 months ago

Last modified 5 months ago

#25024 closed enhancement (implemented)

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

Reported by: fristonio Owned by: alison
Priority: Low Milestone: Tor: 0.3.4.x-final
Component: Core Tor/Tor Version:
Severity: Minor Keywords: tor-comment, review-group-34, 034-triage-20180328
Cc: fristonio Actual Points:
Parent ID: Points: 0.5
Reviewer: asn Sponsor:

Description

We should add a spell check to tor makefile somewhat like https://github.com/client9/misspell This ticket is an extension to #23650 https://trac.torproject.org/projects/tor/ticket/23650

Child Tickets

Attachments (1)

0001-Add-spell-check-to-makefile-to-check-for-typos-in-co.patch (1.8 KB) - added by fristonio 8 months ago.
Misspell integraion with makefile to check for typos.

Download all attachments as: .zip

Change History (34)

comment:1 Changed 8 months ago by fristonio

@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 ?

comment:2 Changed 8 months ago by nickm

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.

comment:3 Changed 8 months ago by fristonio

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?

comment:4 Changed 8 months ago by nickm

Yes; that sounds fine.

comment:5 Changed 8 months ago by fristonio

Ok, I will start working on it then.

comment:6 Changed 8 months ago by 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.

Changed 8 months ago by fristonio

Misspell integraion with makefile to check for typos.

comment:7 Changed 8 months ago by teor

Component: CommunityCore Tor/Tor
Keywords: tor-comment added
Milestone: Tor: 0.3.4.x-final
Points: 0.5
Status: newneeds_revision

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)

comment:8 Changed 8 months ago by teor

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

comment:9 Changed 8 months ago by fristonio

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.

comment:10 Changed 8 months ago by fristonio

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

comment:11 Changed 8 months ago by teor

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

Maybe you need to add a remote called "github", that points to git@…: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/

comment:12 Changed 8 months ago by fristonio

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

comment:13 Changed 8 months ago by teor

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.

comment:14 Changed 8 months ago by fristonio

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.

comment:15 Changed 7 months ago by teor

Status: needs_revisionneeds_review

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.

comment:16 Changed 7 months ago by nickm

Keywords: review-group-34 added

comment:17 Changed 7 months ago by dgoulet

Reviewer: asn

Reviewer week 03/19th

comment:18 Changed 6 months ago by 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.

comment:19 Changed 6 months ago by nickm

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

comment:20 Changed 6 months ago by asn

Status: needs_reviewneeds_revision

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 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? :)

comment:21 Changed 6 months ago by 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?

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.

comment:22 in reply to:  21 Changed 6 months ago by asn

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?

comment:23 Changed 6 months ago by nickm

Keywords: 034-triage-20180328 added

comment:24 Changed 6 months ago by nickm

Keywords: 034-removed-20180328 added

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

comment:25 Changed 6 months ago by fristonio

Keywords: 034-removed-20180328 removed

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. :)

comment:26 Changed 6 months ago by asn

Status: needs_revisionmerge_ready

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 :)

comment:27 Changed 6 months ago by nickm

Status: merge_readyneeds_revision

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.

comment:29 Changed 5 months ago by fristonio

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.

Last edited 5 months ago by fristonio (previous) (diff)

comment:30 Changed 5 months ago by teor

Status: needs_revisionneeds_review

comment:31 Changed 5 months ago by asn

Status: needs_reviewmerge_ready

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.

comment:32 Changed 5 months ago by nickm

Resolution: implemented
Status: merge_readyclosed

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

comment:33 Changed 5 months ago by fristonio

asn, nickm thanks for your help and feedback.

Note: See TracTickets for help on using tickets.