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

Simpler check-typo #1910

Merged
merged 12 commits into from Oct 19, 2018

Conversation

Projects
None yet
4 participants
@gasche
Copy link
Member

commented Jul 17, 2018

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 gasche requested a review from dra27 Jul 17, 2018

@gasche

This comment has been minimized.

Copy link
Member Author

commented Jul 17, 2018

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

@gasche gasche force-pushed the gasche:simpler-check-typo branch 2 times, most recently from 3098fd1 to 6a7dcd0 Jul 17, 2018

@damiendoligez damiendoligez self-requested a review Jul 20, 2018

@dra27

This comment has been minimized.

Copy link
Contributor

commented Jul 24, 2018

@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

This comment has been minimized.

Copy link
Member Author

commented Jul 27, 2018

@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

This comment has been minimized.

Copy link
Contributor

commented Jul 27, 2018

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

This comment has been minimized.

Copy link
Member Author

commented Jul 27, 2018

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

@gasche

This comment has been minimized.

Copy link
Member Author

commented Aug 15, 2018

(gentle ping)

@gasche

This comment has been minimized.

Copy link
Member Author

commented Sep 6, 2018

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

@dra27
Copy link
Contributor

left a comment

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?

Show resolved Hide resolved .gitattributes
Show resolved Hide resolved .gitattributes Outdated
Show resolved Hide resolved .gitattributes Outdated
Show resolved Hide resolved .gitattributes
Show resolved Hide resolved .gitattributes Outdated
Show resolved Hide resolved .gitattributes Outdated
Show resolved Hide resolved 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 comment has been minimized.

Copy link
@dra27

dra27 Sep 6, 2018

Contributor

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

This comment has been minimized.

Copy link
@gasche

gasche Oct 17, 2018

Author Member

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

Show resolved Hide resolved tools/check-typo Outdated
Show resolved Hide resolved tools/check-typo Outdated
@dra27
Copy link
Contributor

left a comment

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
Copy link
Member

left a comment

Looks good!

Show resolved Hide resolved .gitattributes
Show resolved Hide resolved .gitattributes Outdated
Show resolved Hide resolved .gitattributes Outdated
Show resolved Hide resolved .gitattributes Outdated
@dra27

This comment has been minimized.

Copy link
Contributor

commented Oct 17, 2018

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 gasche force-pushed the gasche:simpler-check-typo branch from 54b43fd to de2e82e Oct 17, 2018

@gasche

This comment has been minimized.

Copy link
Member Author

commented Oct 17, 2018

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

This comment has been minimized.

Copy link
Member Author

commented Oct 17, 2018

@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

This comment has been minimized.

Copy link
Contributor

commented Oct 17, 2018

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

@gasche gasche force-pushed the gasche:simpler-check-typo branch 2 times, most recently from a5755c7 to e28d05e Oct 17, 2018

@gasche

This comment has been minimized.

Copy link
Member Author

commented Oct 17, 2018

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

@gasche gasche force-pushed the gasche:simpler-check-typo branch from e28d05e to b263802 Oct 17, 2018

@dra27
Copy link
Contributor

left a comment

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

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

This comment has been minimized.

Copy link
@dra27

dra27 Oct 17, 2018

Contributor
Suggested change
testsuite/tests/typing-unboxed-types/test.ml.reference* typo.white-at-eof
@gasche

This comment has been minimized.

Copy link
Member Author

commented Oct 17, 2018

@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

This comment has been minimized.

Copy link
Member Author

commented Oct 18, 2018

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
Copy link
Member

left a comment

Looks OK except for the unrelated changes.

@@ -2,8 +2,8 @@
include unix
modules = "tscanf2_io.ml"
files = "tscanf2_slave.ml"
reference = "${test_source_directory}/reference"
files = "tscanf2_worker.ml"

This comment has been minimized.

Copy link
@damiendoligez

damiendoligez Oct 18, 2018

Member

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

@gasche gasche force-pushed the gasche:simpler-check-typo branch from b263802 to 2c5bde0 Oct 18, 2018

gasche and others added some commits Jul 17, 2018

rework the relation between .gitattributes and ./tools/check-typo
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`
reorder the testsuite/tests/lib-scanf-2 test structure
The initial motivation is to have a `foo.reference` file
instead of `reference`.
Make typo.very-long-line => typo.long-line=may
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

This comment has been minimized.

Copy link
Contributor

commented Oct 18, 2018

@gasche

This comment has been minimized.

Copy link
Member Author

commented Oct 18, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Oct 18, 2018

@damiendoligez

This comment has been minimized.

Copy link
Member

commented Oct 19, 2018

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

@damiendoligez

This comment has been minimized.

Copy link
Member

commented Oct 19, 2018

Done.

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

1 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gasche

This comment has been minimized.

Copy link
Member Author

commented Oct 19, 2018

Thanks! Merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.