COIN-OR::LEMON - Graph Library

Opened 11 years ago

Closed 11 years ago

#138 closed enhancement (fixed)

Code checker hook script

Reported by: Balazs Dezso Owned by: Balazs Dezso
Priority: major Milestone:
Component: other Version:
Keywords: Cc:
Revision id:

Description (last modified by Balazs Dezso)

The [e7a4f5ef2250] contains a script to check code conformance, which can be used as commit hook in hg. It also contains a minor improvement in unify-sources script.

Attachments (11)

e7a4f5ef2250.patch (3.9 KB) - added by Balazs Dezso 11 years ago.
00e953d56f15.patch (4.0 KB) - added by Balazs Dezso 11 years ago.
043f5a75c230.patch (1.8 KB) - added by Peter Kovacs 11 years ago.
also check Makefile.am files
a8f15f9907db.patch (4.4 KB) - added by Balazs Dezso 11 years ago.
scripts_158ef3b71f5c.patch (5.2 KB) - added by Peter Kovacs 11 years ago.
scripts_1437eaaa5a5b.patch (5.7 KB) - added by Peter Kovacs 11 years ago.
Updated version of Balazs's original changeset [00e953d56f15]
scripts_80edda6f89cd.patch (4.4 KB) - added by Peter Kovacs 11 years ago.
Improvements (also check & unify Makefile.am files correctly)
scripts_f0ce608e320e.patch (8.7 KB) - added by Peter Kovacs 11 years ago.
e05633b02e40.patch (9.2 KB) - added by Balazs Dezso 11 years ago.
Improved unifier script
improvement1_cdbff91c2166.patch (3.2 KB) - added by Peter Kovacs 11 years ago.
Small improvements for [e05633b02e40]
improvement2_d900fd1e760f.patch (2.7 KB) - added by Peter Kovacs 11 years ago.
Print line numbers (depends on [cdbff91c2166])

Download all attachments as: .zip

Change History (42)

Changed 11 years ago by Balazs Dezso

Attachment: e7a4f5ef2250.patch added

comment:1 Changed 11 years ago by Balazs Dezso

Description: modified (diff)

comment:2 Changed 11 years ago by Balazs Dezso

The [00e953d56f15] patch contains a cleaned version of the commit. Please apply it instead of the old one.

Changed 11 years ago by Balazs Dezso

Attachment: 00e953d56f15.patch added

comment:3 Changed 11 years ago by Peter Kovacs

I cannot import [00e953d56f15] with --exact.

comment:4 in reply to:  3 ; Changed 11 years ago by Peter Kovacs

Replying to kpeter:

I cannot import [00e953d56f15] with --exact.

Otherwise I like this script, it helps me a lot.

Btw. shouldn't these two scripts check the Makefile.am files, too? (Of course they should not have header.) I think it would be important, since there were some trouble with whitespace changes in these files, too.

Changed 11 years ago by Peter Kovacs

Attachment: 043f5a75c230.patch added

also check Makefile.am files

comment:5 in reply to:  4 Changed 11 years ago by Peter Kovacs

Replying to kpeter:

Btw. shouldn't these two scripts check the Makefile.am files, too? (Of course they should not have header.) I think it would be important, since there were some trouble with whitespace changes in these files, too.

[043f5a75c230] makes the scripts check these files, too (except for the header checking).

I suggest to import [00e953d56f15] to the main branch (or a modified variant of it, since it cannot be imported with --exact), because it is a useful tool. If [043f5a75c230] is also accepted then these changesets could be merged into one changeset, and unify_sources.sh should be applied to the repository.

Changed 11 years ago by Balazs Dezso

Attachment: a8f15f9907db.patch added

comment:6 Changed 11 years ago by Balazs Dezso

The [a8f15f9907db] is a merge of the previous two patches.

comment:7 in reply to:  6 ; Changed 11 years ago by Alpar Juttner

Replying to deba:

The [a8f15f9907db] is a merge of the previous two patches.

There are tho issues here.

  1. The executable flag is missing from scripts/check-sources.sh
  2. scripts/check-sources.sh is not compatible with scripts/unify-sources.sh. If I run scripts/check-sources.sh on [a8f15f9907db] it says everything is ok, but scripts/unify-sources.sh does several changes at the same time.

comment:8 in reply to:  7 ; Changed 11 years ago by Peter Kovacs

Replying to alpar:

  1. The executable flag is missing from scripts/check-sources.sh

I can't create such a patch that contains this information. If I add the executable flag and export the changes, the created patch can't be imported with --exact.

  1. scripts/check-sources.sh is not compatible with scripts/unify-sources.sh. If I run scripts/check-sources.sh on [a8f15f9907db] it says everything is ok, but scripts/unify-sources.sh does several changes at the same time.

This is because the two scripts are designed to use for different purposes. check-sources.sh is mainly for using as a hg hook, that is why it checks only the modified files when called without an argument. However [3701beec445a] contains an improved version, in which check-sources.sh can be called with the --all (or -a) option to check all the sources files as unify-sources.sh does.

comment:9 in reply to:  8 ; Changed 11 years ago by Alpar Juttner

Replying to kpeter:

Replying to alpar:

  1. The executable flag is missing from scripts/check-sources.sh

I can't create such a patch that contains this information.

You can, you just probably don't know how. :)

If I add the executable flag and export the changes, the created patch can't be imported with --exact.

The --git option is your friend. I strongly suggest to use it by default by placing something like this into ~/.hgrc.

[diff]
git=1
[defaults]
qimport = --git
qrefresh = --git

This is from my own hg config file. It may be that [diff] section is enough for everything, I haven't checked it.

comment:10 in reply to:  4 ; Changed 11 years ago by Alpar Juttner

Replying to kpeter:

Btw. shouldn't these two scripts check the Makefile.am files, too?

Makefile.am files should not be checked/modified in this way. The tabs in Makefiles are mandatory and should not be replaced, even if GNU's make allows to do that.

Changed 11 years ago by Peter Kovacs

Attachment: scripts_158ef3b71f5c.patch added

comment:11 in reply to:  9 Changed 11 years ago by Peter Kovacs

Replying to alpar:

The --git option is your friend. I strongly suggest to use it by default by placing something like this into ~/.hgrc.

Thank you. [158ef3b71f5c] is the fixed patch.

comment:12 in reply to:  10 Changed 11 years ago by Peter Kovacs

Replying to alpar:

Makefile.am files should not be checked/modified in this way. The tabs in Makefiles are mandatory and should not be replaced, even if GNU's make allows to do that.

I understand, but the unification is necessary in any case. See e.g. lemon/Makefile.am. TABs should be replaced to 8 spaces or 8 spaces should be replaced to TABs.

comment:13 in reply to:  8 ; Changed 11 years ago by Balazs Dezso

Replying to kpeter:

Replying to alpar:

  1. The executable flag is missing from scripts/check-sources.sh

I can't create such a patch that contains this information. If I add the executable flag and export the changes, the created patch can't be imported with --exact.

  1. scripts/check-sources.sh is not compatible with scripts/unify-sources.sh. If I run scripts/check-sources.sh on [a8f15f9907db] it says everything is ok, but scripts/unify-sources.sh does several changes at the same time.

This is because the two scripts are designed to use for different purposes. check-sources.sh is mainly for using as a hg hook, that is why it checks only the modified files when called without an argument. However [3701beec445a] contains an improved version, in which check-sources.sh can be called with the --all (or -a) option to check all the sources files as unify-sources.sh does.

Fortunately, the check-sources.sh checks the commited files, not all the files. This is the correct behaviour of the hook. In the script file there is a comment about how the script should be used. The executable flag should be added.

Unfortnately, the hgrc cannot be copied automically with clone (for example if I make a local clone, and I would like to add the same hgrc to each lemon repository, but there are no hooks for local clones), so the hook config should be write into each repository.

comment:14 in reply to:  13 ; Changed 11 years ago by Peter Kovacs

Replying to deba:

Unfortnately, the hgrc cannot be copied automically with clone (for example if I make a local clone, and I would like to add the same hgrc to each lemon repository, but there are no hooks for local clones), so the hook config should be write into each repository.

Another option is to write the hook config into the "global" .hgrc file, so it is used for all local repositories.

comment:15 in reply to:  14 Changed 11 years ago by Alpar Juttner

Replying to kpeter:

Another option is to write the hook config into the "global" .hgrc file, so it is used for all local repositories.

Which is an option only if you never use hg anything else but for LEMON development. I would suggest the following instead.

  1. Setup a repository called out for your outgoing changesets and install the script on incoming (and not the commit) hook of this repo.
  2. Whenever you starts working on something, you clone the main branch (of which you should also keep a local copy)
  3. Instead of directly committing, use MQ for creating changesets, for it makes it easy to modify them later.
  4. When you have finished your work, push it to the outgoing repo. It is fails, edit the wrong files, refresh the changeset with hg qref and push it again.

comment:16 Changed 11 years ago by Peter Kovacs

What should we do with this ticket? Alpar do not like replacing TABs with 8 spaces in Makefile.am files. However unification would be necessary in these files, too. See e.g. lemon/Makefile.am.

So every 8 spaces should be replaced with TABs or all TABs should be replaced with 8 spaces.

I think we have to options:

  • unify these files manually or
  • extend the unification script to these files.

comment:17 in reply to:  16 Changed 11 years ago by Alpar Juttner

Replying to kpeter:

What should we do with this ticket? Alpar do not like replacing TABs with 8 spaces in Makefile.am files.

This is not a matter of taste but it is because a standard makefile requires TABs not spaces.

In general I support unification, but I do not consider it very urgent. If we have patch for the hook script and the unifier that also handles Makefile, that is nice, but I don't think it is a blocker of release 1.0.

Changed 11 years ago by Peter Kovacs

Attachment: scripts_1437eaaa5a5b.patch added

Updated version of Balazs's original changeset [00e953d56f15]

Changed 11 years ago by Peter Kovacs

Attachment: scripts_80edda6f89cd.patch added

Improvements (also check & unify Makefile.am files correctly)

comment:18 Changed 11 years ago by Peter Kovacs

I cleaned the script files. They check and unify the Makefile.am files according to Alpar's notes.

Please review [1437eaaa5a5b] and [80edda6f89cd]. If they will be applied, I also suggest to run the unification script on the repository (it will unify Makefile.am files and two header files). Maybe it would be better before the release of Lemon 1.0.

comment:19 Changed 11 years ago by Peter Kovacs

What do you think about [1437eaaa5a5b] and [80edda6f89cd]?

comment:20 in reply to:  19 ; Changed 11 years ago by Alpar Juttner

Replying to kpeter:

What do you think about [1437eaaa5a5b] and [80edda6f89cd]?

Here are my comments

  • I think it's a bad idea to have too scripts for unifying the source. It doubles the maintenance work and it is also error prone. The checker script should be just an option of the unify script. Currently it first creates a unified version of the source files in a temp file and then overwrites the original one with it. For checking the source, it would be enough to diff these files instead of overwriting.
  • Unfortunately long lines are sometimes unavoidable in makefiles and in doxygen documentations, thus these files should not be checked for long lines.

Changed 11 years ago by Peter Kovacs

Attachment: scripts_f0ce608e320e.patch added

comment:21 in reply to:  20 ; Changed 11 years ago by Peter Kovacs

Replying to alpar:

Here are my comments

  • I think it's a bad idea to have too scripts for unifying the source. It doubles the maintenance work and it is also error prone. The checker script should be just an option of the unify script. Currently it first creates a unified version of the source files in a temp file and then overwrites the original one with it. For checking the source, it would be enough to diff these files instead of overwriting.
  • Unfortunately long lines are sometimes unavoidable in makefiles and in doxygen documentations, thus these files should not be checked for long lines.

[f0ce608e320e] contains a reworked version according to your comments. For more information run the script with --help option.

comment:22 in reply to:  21 ; Changed 11 years ago by Balazs Dezso

Replying to kpeter:

Replying to alpar:

Here are my comments

  • I think it's a bad idea to have too scripts for unifying the source. It doubles the maintenance work and it is also error prone. The checker script should be just an option of the unify script. Currently it first creates a unified version of the source files in a temp file and then overwrites the original one with it. For checking the source, it would be enough to diff these files instead of overwriting.
  • Unfortunately long lines are sometimes unavoidable in makefiles and in doxygen documentations, thus these files should not be checked for long lines.

[f0ce608e320e] contains a reworked version according to your comments. For more information run the script with --help option.

I would like to suggest some improvements on this patch.

  • In my point of view, that which files are used and what we do with the files are two independent things, therefore the parameters should be separate
  • The different file selection methods could be the following, all files, the modified files in the repo, the given files in command line, and the commited files just for hook usage.
  • The effect could be modification, check, and check with long line query.
  • Do not use magic numbers in scripts, use rather strings.

comment:23 in reply to:  22 ; Changed 11 years ago by Peter Kovacs

Replying to deba:

I would like to suggest some improvements on this patch.

I agree with your ideas. But could you please help me implementing them?

comment:24 Changed 11 years ago by Alpar Juttner

Replying to deba:

  • In my point of view, that which files are used and what we do with the files are two independent things, therefore the parameters should be separate

I agree.

  • The different file selection methods could be the following, all files, the modified files in the repo, the given files in command line, and the commited files just for hook usage.

These flags can be -a, -m and nothing (i.e. the files are explicitly given), respectively.

  • The effect could be modification, check, and check with long line query.

I suggest a -n or --dry-run option to change nothing just checking.

The long lines should only be checked on .h and .cc files, and they should always be checked on them IMHO. So an option for that is not so important. Also long lines cannot be broken automatically, but they should be reported, of course.

  • Do not use magic numbers in scripts, use rather strings.

What do "magic numbers" mean here?

comment:25 in reply to:  23 ; Changed 11 years ago by Alpar Juttner

Replying to kpeter:

I agree with your ideas. But could you please help me implementing them?

I will do that next week. (Assuming that I will learn what are "magic numbers" we want to get rid of.)

comment:26 in reply to:  25 Changed 11 years ago by Peter Kovacs

Replying to alpar:

I will do that next week. (Assuming that I will learn what are "magic numbers" we want to get rid of.)

Thank you.

Magic numbers: I think it refers to the usage of the CHECK_MODE variable I introduced. This variable can have values of {0,1,2}.

comment:27 in reply to:  24 ; Changed 11 years ago by Peter Kovacs

Replying to alpar:

  • The different file selection methods could be the following, all files, the modified files in the repo, the given files in command line, and the commited files just for hook usage.

These flags can be -a, -m and nothing (i.e. the files are explicitly given), respectively.

And what about checking only the committed files? I suggest --all|-a, --modified|-m, --committed|-c, and nothing for the files that are given as command line arguments.

  • The effect could be modification, check, and check with long line query.

I suggest a -n or --dry-run option to change nothing just checking.

A prefer --nomodify|-n.

comment:28 in reply to:  27 Changed 11 years ago by Alpar Juttner

Replying to kpeter:

I suggest a -n or --dry-run option to change nothing just checking.

A prefer --nomodify|-n.

--dry-run notation is quite widely used, see e.g. svn help merge

Changed 11 years ago by Balazs Dezso

Attachment: e05633b02e40.patch added

Improved unifier script

comment:29 Changed 11 years ago by Balazs Dezso

The [e05633b02e40] contains the improved unifier and checker script.

Changed 11 years ago by Peter Kovacs

Small improvements for [e05633b02e40]

Changed 11 years ago by Peter Kovacs

Print line numbers (depends on [cdbff91c2166])

comment:30 in reply to:  29 ; Changed 11 years ago by Peter Kovacs

Replying to deba:

The [e05633b02e40] contains the improved unifier and checker script.

I really like it. This version is much better than all of the older ones! The new --interactive|-i and --werror|-w options are also useful.

I just made two changesets to improve it:

  • [cdbff91c2166] contains small improvements.
  • [d900fd1e760f] adds line number display ability for the checking mode, since I missed it in the new implementation.

comment:31 in reply to:  30 Changed 11 years ago by Alpar Juttner

Resolution: fixed
Status: newclosed

Replying to kpeter:

I just made two changesets to improve it:

  • [cdbff91c2166] contains small improvements.
  • [d900fd1e760f] adds line number display ability for the checking mode, since I missed it in the new implementation.

Meanwhile I also made some improvements, see [0fbbb4bc42dd]. All of them are in the main branch now.

Note: See TracTickets for help on using tickets.