Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

[PATCH] 1.7.2 code: Use standard -s (short) and --long option syntax #16

Closed
jaalto opened this Issue Mar 31, 2012 · 7 comments

Comments

Projects
None yet
2 participants
Contributor

jaalto commented Mar 31, 2012

Here is patch for 1.7.2 to use standard option syntax. For in-depth details, see: http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=602585

 git remote add jaalto git://github.com/jaalto/improved-pngquant.git
 git fetch jaalto
 git diff jaalto/options

 git merge jaalto/options
Contributor

jaalto commented Mar 31, 2012

Here is patch for the manpage:

git remote add jaalto git://github.com/jaalto/improved-pngquant.git
git fetch jaalto
git diff jaalto/manpage

git merge jaalto/manpage
Owner

pornel commented Mar 31, 2012

Thanks. I think it's a very good idea to migrate to standard option syntax. However, I'm worried that it breaks backward compatibility.

It's very likely that people have scripts using options with a single hyphen, and pngquant shouldn't suddenly start rejecting those. For a while both -foo and --foo should be supported to help people migrate.

With standard behavior joining of multiple single-letter options is allowed, e.g. -v -f should be same as -vf. Currently pngquant's homegrown argument parsing doesn't allow that.

Do you know if it would be possible to use a standard library call for argument parsing (getopts_long or similar) and still support "legacy" pngquant option switches?

Contributor

jaalto commented Mar 31, 2012

On 2012-03-31 08:15, porneL wrote:

| Thanks. I think it's a very good idea to migrate to standard option
| syntax. However, I'm worried that it breaks backward compatibility.
|
| It's very likely that people have scripts using options with a
| single hyphen, and pngquant shouldn't suddenly start rejecting
| those. For a while both -foo and --foo should be supported to
| help people migrate.

If you wish, you can make people to migrate to new syntax by:
displaying a message for every "old" option:

"-option is obsoleted and will be removed, please migrate to --option"

Personally, I think that's too much work for such a small program. I'd simply
have the program exit with message:

 "unknown option"

and have people figure out that the new options are using --long
format. It's relatively easy to fix scripts.

| With standard behavior joining of multiple single-letter options is
| allowed, e.g. -v -f should be same as -vf. Currently pngquant's
| homegrown argument parsing doesn't allow that.
|
| Do you know if it would be possible to use standard library call for
| argument parsing (getopts_long or similar) and still support
| "legacy" pngquant option switches?

There is GNU option library "getopt" that is the "de facto" of
handling options. See:

 http://www.gnu.org/software/libc/manual/html_node/Getopt-Long-Options.html

Make your code depends on that and your option handling can be greatly
simplified.

Jari

Owner

pornel commented Mar 31, 2012

This tool is over 10 years old, and I'd like newer version to be included in Debian, so I think a sudden breaking change is not appropriate.

Would you be able to change pngquant to use getopt and keep existing options with a warning? (I suppose you'd have to preprocess argc/argv to fix single-hyphen names)

Contributor

jaalto commented Apr 1, 2012

On 2012-03-31 13:46, porneL wrote:
|
| This tool is over 10 years old, and I'd like newer version to be
| included in Debian, so I think a sudden breaking change is not
| appropriate.

Hi,

Actually those patches (long options) are planned to be included in
next upload to Debian. Any user that has been using old options will
need to adjust their scripts.

I sent the patches here to be included in the official sources.

| Would you be able to change pngquant to use getopt and keep existing
| options with a warning? (I suppose you'd have to preprocess
| argc/argv to fix single-hyphen names)

OBSOLETING OPTIONS

I added the check for old options. Please pull from branch:

jaalto/options

c09d143 2012-04-01 pngquant.c: Check obsolete options
da3e76b 2012-03-31 Standard option syntax: -o (short), --option (long)

GNU GETOPT SUPPORT

Unfortunately I do not have time to add getopt functionality. But I'm
sure there is penty of examples about getopt if you Google around.

Thanks for including this to new release,
Jari

Owner

pornel commented Apr 1, 2012

Thanks. I've applied those patches.

I've noticed small issue: options are compared using str*n*cmp, so they allow prefix. -nof option used to be equivalent to -noforce.

With the extra hyphen now --no is ambiguous. I'm going to drop that prefix support completely and use exact strcmp for options …unless that prefix handling was a standard behavior?

Contributor

jaalto commented Apr 1, 2012

On 2012-04-01 06:08, porneL wrote:
| Thanks. I've applied those patches.
|
| With the extra hyphen now --no is ambiguous. I'm going to drop
| that prefix support completely and use exact strcmp for options
| ...unless that prefix handling was a standard behavior?

Dropping "no" prefix sounds good plan to me.

Thanks,
Jari

@pornel pornel closed this Apr 1, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment