Conversation
9f86703 to
8afb771
Compare
dra27
left a comment
There was a problem hiding this comment.
Firstly, thanks for doing this, because I haven't got to writing it!
Given that this is a script for developers, I think it better to have a version which maybe wants more features in the future than spend ages without one.
It's worth fixing the couple of issues I've highlighted - longer term, doing git diff against trunk will cause the script to scan files which have been changed by upstream changes - it would be better (I think) to identify the merge-base with the branch specified and scan the commits inbetween for the list of files - which is what travis-ci.sh does.
493852b to
24eed32
Compare
|
I have rebased this PR on top of #1910, and addressed the review comments -- but I think that another bashism-detection-check would not hurt. |
24eed32 to
527d209
Compare
|
(Not sure why this was automatically closed, but I need to rebase it on trunk now that #1910 is in.) |
527d209 to
622f782
Compare
|
I just rebased this PR on top of trunk; reviews are warmly welcome. (But then again, this is not high priority.) |
622f782 to
46951bb
Compare
46951bb to
cf4e672
Compare
|
Gentle ping: given that all issues of the previous review round have been addressed, could we accept this PR? |
Before this change, check-typo would run on manual/Makefile for example, while this file lives within a pruned directory so it ought to be ignored by the tool. Note: the check-typo code seems to assume that the only pruned things are directory, it prints "pruned directory ..." when something is pruned. I haven't changed this part of the logic; but note that normal ./check-typo invocation will only check pruning for directories.
this comment special-cases the prune-detection logic to use the `git check-attr` layer directly, instead of using the convenience function `get_attrs ..` which parses its output. On my machine, calling --check-prune on the testsuite files goes from 17s to 12s when this patch is applied.
./tools/check-typo-since trunk ./tools/check-typo-since HEAD~10 In most cases, this should be much faster than running check-typo on the whole directory.
damiendoligez
left a comment
There was a problem hiding this comment.
Looks good to me, but I'll let @dra27 have another look.
|
Ping :-) |
|
@damiendoligez (or @dra27) would you approve the PR? The feature is useful and the diff touches non-critical part of the codebase. |
|
Sorry for the delay getting back to this one <pages out m4sh and pages back in awk> ... LGTM! |
* prepend opam exec -- on all dune commands * mention that opam exec -- can be omitted in certain cases
I have not learned to use the check-typo pre-commit-hook yet (and I think there are various situations where I want to run check-typo at other times than at pre-commits), but I already find just running
./tools/check-typo(to check the whole repository) very slow, it takes slightly above one minute on my machine.This PR proposes an auxiliary tool,
check-typo-since, which takes a git reference as argument, and runscheck-typoon all the files that have changed since this reference -- due to further commit, or not-yet-committed changes. The following could be usefulI first considered adding this feature to
check-typodirectly (--since trunk), but the control flow ofcheck-typois horrible spaghetti and I don't want to touch it.To make
check-typo-sincework well, I had to make some tweaks to.gitattributes. In particular, instead of having/manual ocaml-typo=prune, which tells that themanualdirectory is to be pruned, I changed it into/manual** ocaml-typo=prune, which tells thatmanualand any file underneath is to be pruned. This makes the implementation ofcheck-typo-sincesimple and easy, but one has to remember to always use the child-closure operator**when pruning in gitattributes.(I realize now that some of the logic is very close to the code of the pre-commit-hook, but that I made different choices which could also be used to simplify the pre-commit-hook's logic, if @dra27 agrees that they are sensible choices.)