Simpler check-typo by gasche · Pull Request #1910 · ocaml/ocaml

gasche · 2018-07-17T09:23:36Z

I wanted this to fix this particular comment of @dra27 in the review of #1905. If the present PR is accepted, I will rebase #1905 on top of it.

gasche · 2018-07-17T09:41:00Z

@dra27 where is the use of check-typo (and maybe the pre-commit-hook) documented? If you have committed any documentation in the repository (I could only find a mention in passing in CONTRIBUTING.md), I think that it would be nice to have something in HACKING.adoc -- either the toplevel one, or a new tools/HACKING.adoc.

dra27 · 2018-07-24T22:24:50Z

@gasche - I'm slightly embarrassed to say that while it's on my TODO list, documentation beyond the mention in CONTRIBUTING is all that I've committed so far.

gasche · 2018-07-27T10:23:57Z

@dra27: I'm hoping that you will eventually review this PR (and later #1905). There is nothing urgent about it, so feel free to prioritize other things.

dra27 · 2018-07-27T12:54:52Z

I had started to review it! I think the changes are great, just that going through it was taking time - I'll definitely come to it when I'm back from vacation in the second week of Aug.

gasche · 2018-07-27T12:58:43Z

Thanks for the update, and have nice vacations :-)

gasche · 2018-08-15T21:09:50Z

(gentle ping)

gasche · 2018-09-06T06:25:48Z

Would maybe someone else be interested in looking at this? @nojb, @Octachron, @trefis?

dra27

Sorry for the delay reviewing this. I have reviewed very carefully the changes to .gitattributes and reckon that all the pertinent logic which was hard-coded in check-typo is now transferred to .gitattributes.

The changes to the script itself look good to me - perhaps @damiendoligez could cast a quick eye over it for any concerns about coding style?

.gitattributes

tools/check-typo

dra27 · 2018-09-06T08:23:33Z

tools/check-typo

+  env $OCAML_CT_GIT_INDEX git check-attr --all $OCAML_CT_CA_FLAG "$1" \
+  | grep -o " typo\\..*$" | sed "s/ typo\\.//g" \
+  | grep -v ": unset" | grep -v ": false" \
+  | sed "s/: set//g" | sed "s/: true//g" | sed "s/: may/?/g"


This pipe can be more simply (or at least compactly): sed -ne '/: $set\|true\|may$/s/.* typo\.//p' -e 's/: $set\|true$//' -e 's/:.*/?/'

My sed-fu is not high enough to understand your version, so I'll stick with dumb pipes for now.

tools/check-typo

dra27

Sorry for the delay reviewing this. I have reviewed very carefully the changes to .gitattributes and reckon that all the pertinent logic which was hard-coded in check-typo is now transferred to .gitattributes.

The changes to the script itself look good to me - perhaps @damiendoligez could cast a quick eye over it for any concerns about coding style?

damiendoligez

Looks good!

.gitattributes

dra27 · 2018-10-17T08:53:33Z

I just ran the script over the entire repo and found that the yacc/*.[ch] was triggering unused warnings (because it was typo.long-line rather than typo.long-line=may).

I've also pushed a commit which makes it so that typo.very-long-line implies typo.long-line=may.

This needs rebasing, so feel free to squash the first commit and potentially drop the latter commit when you do!

gasche · 2018-10-17T09:38:27Z

I rebased the PR and I'm in the process of going over the review comments. I hope to finish today to get this PR dealt with.

gasche · 2018-10-17T09:43:07Z

@dra27 I'm sorry, I am just seeing that you did pushes on your own. (I force-pushed when I rebased.) I fixed the yacc-stuff as well, but I'm happy to integrate the long-line/very-long-line commit in the PR, could you push it again?

dra27 · 2018-10-17T10:19:31Z

No apology needed - it's your branch 🙂 Commit re-pushed...

gasche · 2018-10-17T12:30:42Z

I believe that I should now have addressed all review comments (thanks!).

dra27

One tweak - though mainly a chance to use GitHub's new suggest edit feature :)

dra27 · 2018-10-17T14:03:15Z

.gitattributes

+testsuite/tests/lib-unix/win-stat/fakeclock.c           typo.missing-header=false
+testsuite/tests/misc-unsafe/almabench.ml                typo.long-line
+testsuite/tests/tool-toplevel/strings.ml                typo.utf8
+testsuite/tests/typing-unboxed-types/test.ml.reference* typo.white-at-eof


Suggested change

testsuite/tests/typing-unboxed-types/test.ml.reference* typo.white-at-eof

gasche · 2018-10-17T14:13:52Z

@dra27: the way I read the diff produced by this indeed-intriguing feature, you are propose to remove the reference.ml* white-at-eof line. But if I remove it on my local copy, then I do get an alarm:

$ ./tools/check-typo testsuite/tests/typing-unboxed-types/test.ml.reference*
testsuite/tests/typing-unboxed-types/test.ml.reference-flat:213.1: [white-at-eof] empty line(s) at EOF
testsuite/tests/typing-unboxed-types/test.ml.reference-noflat:179.1: [white-at-eof] empty line(s) at EOF

What's going on here?

gasche · 2018-10-18T11:34:16Z

In the interest of not waiting too much on this issue (its discussion in #2096 (comment) bumped it up in my priority list), I'm thinking of merging this PR today or tomorrow.

damiendoligez

Looks OK except for the unrelated changes.

damiendoligez · 2018-10-17T15:04:09Z

testsuite/tests/lib-scanf-2/tscanf2_master.ml

 modules = "tscanf2_io.ml"
-files = "tscanf2_slave.ml"
-reference = "${test_source_directory}/reference"
+files = "tscanf2_worker.ml"


These changes are completely unrelated to check-typo (unless you intend to add a style checker). They should be in a separate PR (and probably touch many more files).

Before this patch, check-typo is directed by a single git attribute, ocaml-typo, which is used as a key to set a value: ocaml-typo=long-line,missing-header (the value here is `long-line,missing-header`, and the code splits the comma later) This model is very fragile because .gitattributes does not allow to give attribute keys a collecting/aggregating semantic: each new setting of the key removes the previous setting, instead of adding to them. For example, testsuite/tests/** ocaml-typo=missing-header testsuite/tests/win-unicode/*.ml ocaml-typo=utf8 and testsuite/tests/win-unicode/*.ml ocaml-typo=utf8 testsuite/tests/** ocaml-typo=missing-header are not equivalent, and instead of using either one we would introduce redundancy for robustness: testsuite/tests/** ocaml-typo=missing-header testsuite/tests/win-unicode/*.ml ocaml-typo=missing-header,utf8 With this patch, we switch to a model where each ocaml-typo setting is its own attribute, of the form `typo.<<attribute>>`. The lines above would be written, in either order: testsuite/tests/** typo.missing-header testsuite/tests/win-unicode/*.ml typo.utf8 Not only does this approach make our .gitattributes more robust, it allows for a more fine-grained treatment of the "unused-prop" marker. This was used as an attribute to say: don't make it in an error if the given typo-rule is in fact respected (by default, opting out of a typo-rule gives an error if the typo-rule is respected). But because of the single-key nature of ocaml-typo, unused-prop would range over all settings, not just one of them. For example emacs/caml.el ocaml-typo=long-line,unused-prop,missing-header seems to suggest that 'unused-prop' only qualifies the 'long-line' rule, but in fact it also ranges over 'missing-header'. In contrast, with this patch, we write the following: emacs/caml.el typo.long-line=may typo.missing-header the `=may` value setting is used to make an exception to a typo-rule optional. Interestingly, most .gitattributes lines worked without extra error when I turned each unused-prop in a =may setting over the rule just before, instead of all rules: our checking is now more precise than before, better capturing the intent of the .gitattributes author. As I had to rewrite parts of the check-typo code for this, I took the opportunity to rename a couple variables speaking about SVN (now long defunct) into more meaningful names: - `$is_svn` => `$path_in_index` - `$svnrules` => `$attr_rules`

…honored

…ceptions

The initial motivation is to have a `foo.reference` file instead of `reference`.

typo.long-line can still be explicitly set in .gitattributes (and typo.long-line typo.very-long-line=may will still issue unused if no lines are more than 80 columns)

shindere · 2018-10-18T14:09:08Z

David Allsopp (2018/10/18 07:03 -0700):

You're right, I had! In which case, it should be `typo.prune`, but perhaps this can be simplified by altering the tests. The following files: ``` tests/typing-unboxed-types/test.ml.reference-flat tests/typing-unboxed-types/test.ml.reference-noflat tests/translprim/array_spec.compilers.reference.flat tests/translprim/module_coercion.compilers.reference.flat tests/translprim/module_coercion.compilers.reference.no-flat tests/translprim/array_spec.compilers.reference.no-flat ``` are the only ones which don't end `.reference`. Perhaps they should just be renamed to `.flat.reference` and `.no-flat.reference` so that the normal prune rule comes into play?

Yes, please! Sorry for having done things the wrong way initially!

gasche · 2018-10-18T14:25:31Z

I'm busy with other things and would like to get done with this PR. Is it possible to leave those further suggestions to future PRs?

shindere · 2018-10-18T14:27:48Z

Gabriel Scherer (2018/10/18 07:25 -0700):

I'm busy with other things and would like to get done with this PR. Is it possible to leave those further suggestions to future PRs?

Fine with me :)

damiendoligez · 2018-10-19T13:03:06Z

@gasche just let me push a commit to rename the reference files before you merge.

damiendoligez · 2018-10-19T14:01:56Z

Done.

gasche · 2018-10-19T16:00:43Z

Thanks! Merged.

Co-authored-by: cuihtlauac <cuihtlauac@users.noreply.github.com>

gasche requested a review from dra27 July 17, 2018 09:23

gasche force-pushed the simpler-check-typo branch 2 times, most recently from 3098fd1 to 6a7dcd0 Compare July 18, 2018 22:50

gasche mentioned this pull request Jul 18, 2018

./tools/check-typo-since trunk #1905

Merged

damiendoligez self-requested a review July 20, 2018 13:37

dra27 reviewed Sep 6, 2018

View reviewed changes

damiendoligez approved these changes Sep 13, 2018

View reviewed changes

.gitattributes Show resolved Hide resolved

.gitattributes Outdated Show resolved Hide resolved

.gitattributes Outdated Show resolved Hide resolved

.gitattributes Outdated Show resolved Hide resolved

gasche force-pushed the simpler-check-typo branch from 54b43fd to de2e82e Compare October 17, 2018 09:06

gasche force-pushed the simpler-check-typo branch 2 times, most recently from a5755c7 to e28d05e Compare October 17, 2018 12:28

gasche force-pushed the simpler-check-typo branch from e28d05e to b263802 Compare October 17, 2018 12:33

dra27 reviewed Oct 17, 2018

View reviewed changes

damiendoligez approved these changes Oct 18, 2018

View reviewed changes

gasche force-pushed the simpler-check-typo branch from b263802 to 2c5bde0 Compare October 18, 2018 11:35

check-typo: make (--check-prune .git) work as expected

69a4952

gasche and others added 10 commits October 18, 2018 13:44

check-typo: make sure that 'prune' attributes on non-directories are …

17fa399

…honored

check-typo: simplify implementation by deconstructing the built-in ex…

26cec30

…ceptions

check-typo: remove useless gitattributes

321f219

Changes entry

41991ad

check-typo: silence minor warnings

42abe7b

reorder the testsuite/tests/lib-scanf-2 test structure

8468737

The initial motivation is to have a `foo.reference` file instead of `reference`.

Make typo.very-long-line => typo.long-line=may

af38eb2

typo.long-line can still be explicitly set in .gitattributes (and typo.long-line typo.very-long-line=may will still issue unused if no lines are more than 80 columns)

tools/check-typo: add a a "--get-attrs <path>" mode for debug purposes

a18cee9

address review comments by David and Damien

2c5bde0

gasche mentioned this pull request Oct 18, 2018

In error messages, print the source fragment responsible for the error #2096

Merged

GPR#1910: rename .reference.* files

197f509

gasche merged commit ce17ca1 into ocaml:trunk Oct 19, 2018

dra27 mentioned this pull request Dec 19, 2018

Fix reading of typo.prune in CI #2209

Merged

EmileTrotignon pushed a commit to EmileTrotignon/ocaml that referenced this pull request Jan 12, 2024

[create-pull-request] automated change (ocaml#1910)

6cc2a57

Co-authored-by: cuihtlauac <cuihtlauac@users.noreply.github.com>

dra27 mentioned this pull request May 22, 2025

Minor fix to pre-commit githook #14050

Merged

Conversation

gasche commented Jul 17, 2018

Uh oh!

gasche commented Jul 17, 2018

Uh oh!

dra27 commented Jul 24, 2018

Uh oh!

gasche commented Jul 27, 2018

Uh oh!

dra27 commented Jul 27, 2018

Uh oh!

gasche commented Jul 27, 2018

Uh oh!

gasche commented Aug 15, 2018

Uh oh!

gasche commented Sep 6, 2018

Uh oh!

dra27 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dra27 Sep 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gasche Oct 17, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dra27 left a comment

Choose a reason for hiding this comment

Uh oh!

damiendoligez left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dra27 commented Oct 17, 2018

Uh oh!

gasche commented Oct 17, 2018

Uh oh!

gasche commented Oct 17, 2018

Uh oh!

dra27 commented Oct 17, 2018

Uh oh!

gasche commented Oct 17, 2018

Uh oh!

dra27 left a comment

Choose a reason for hiding this comment

Uh oh!

dra27 Oct 17, 2018

Choose a reason for hiding this comment

Uh oh!

gasche commented Oct 17, 2018

Uh oh!

gasche commented Oct 18, 2018

Uh oh!

damiendoligez left a comment

Choose a reason for hiding this comment

Uh oh!

damiendoligez Oct 17, 2018

Choose a reason for hiding this comment

Uh oh!

shindere commented Oct 18, 2018 via email

Uh oh!

gasche commented Oct 18, 2018

Uh oh!

shindere commented Oct 18, 2018 via email

Uh oh!

damiendoligez commented Oct 19, 2018

Uh oh!

dra27 Sep 6, 2018 •

edited

Loading