beyondgrep / ack2

**ack 2 is no longer being maintained. ack 3 is the latest version.**
https://github.com/beyondgrep/ack3/
Other
1.48k stars 138 forks source link

make: add Makefile.Debug and Makefile.Release #451

Closed mingwandroid closed 10 years ago

mingwandroid commented 10 years ago

.. instead of makefile Makefile GNUmakefile

This generalises Makefile detection with a regex. I needed this because Qt Project creates Makefile.Debug and Makefile.Release, and I think it makes ack more useful out-of-the-box for Qt developers.

petdance commented 10 years ago

I'm not sure I want to handle every possible Makefile in the core ack. Thoughts?

mingwandroid commented 10 years ago

No just the obvious ones like those matching the regex above. For example without this every Qt developer would find ack lacking for searching in makefiles generated by qmake. On Apr 21, 2014 2:31 AM, "Andy Lester" notifications@github.com wrote:

I'm not sure I want to handle every possible Makefile in the core ack. Thoughts?

— Reply to this email directly or view it on GitHubhttps://github.com/petdance/ack2/pull/451#issuecomment-40910474 .

petdance commented 10 years ago

How widespread is Qt? And is it worth adding those rules to ack itself rather than telling Qt developers to use --type-add=make:is:Makefile.Debug and --type-add=make:is:Makefile.Release in their ackrc files?

mingwandroid commented 10 years ago

It seems to be taking over as the go to framework for cross platform C++ development.

KDE is built on it and many historically gtk projects are now ditching that for Qt - wireshark and subsurface for example - this is due to gtk 3 forcing major rewrites and IMHO also that Qt just gathered a lot of momentum while gtk stagnated.

It works on Linux, Windows, Mac, Android, iOS and WinRT and provides both LGPL and commercial licensing options.

.. anyway can you tell me the downsides? I removed 3 fixed patterns replacing them with one pattern that catches a higher proportion of things that are almost definitely makefiles, so I guess there must be some thing really bad with using regexs for file type matching? On Apr 21, 2014 3:15 AM, "Andy Lester" notifications@github.com wrote:

How widespread is Qt? And is it worth adding those rules to ack itself rather than telling Qt developers to use --type-add=make:is:Makefile.Debugand --type-add=make:is:Makefile.Release.

— Reply to this email directly or view it on GitHubhttps://github.com/petdance/ack2/pull/451#issuecomment-40911705 .

petdance commented 10 years ago

can you tell me the downsides?

I don't know that there are any. That's why I'm asking for public commentary.

It's not about regexes being bad. It's about adding more features than we need to.

mingwandroid commented 10 years ago

A feature was not added to support this. Regex matching existed already. The classification of makefiles was expanded to cover more real world scenarios, with the nice side effect of needing less code. On Apr 21, 2014 1:51 PM, "Andy Lester" notifications@github.com wrote:

can you tell me the downsides?

I don't know that there are any. That's why I'm asking for public commentary.

It's not about regexes being bad. It's about adding more features than we need to.

— Reply to this email directly or view it on GitHubhttps://github.com/petdance/ack2/pull/451#issuecomment-40933242 .

n1vux commented 10 years ago

Replies

How widespread is Qt?

Qt is quite widespread in Linux (and resurgent/growing), in cross-platform app development, and (maybe still) in embedded-device GUI. It's an influential community where "ack2 works wonderfully out of the box" would be worth our initial effort at supporting: they are used to providing patches to things they like. Bread cast upon these waters will multiply.

And is it worth adding those rules to ack itself rather than telling Qt developers to use --type-add=make:is:Makefile.Debug and --type-add=make:is:Makefile.Release in their ackrc files?

A doc patch to give Qt devs the --type-add's needed is minimum acceptable response to this bug.

But "just works out of the box" for Qt C devs is properly within our remit. --make should find Makefile.Debug and Makefile.Release, and --nomake (a default in ack1) should ignore them.

Downsides

However. /Makefile.*/ seems at best overbroad. I'm not worried about Makefile.bak & Makefile~ as the .bak suppression should still disqualify them, but i do not want --make to include **Makefile.c** nor --nomake to ignore Makefile.c.

Counterproposal.

m{ Makefile 
   (?: [.] (?: Release | Debug ))? }ix 

is sufficient to answer Qt's identified needs and offers a place to add future special cases.

petdance commented 10 years ago

I don't see a need to complicate it with a regex. I'd just as soon make it

--type-add=make:is:Makefile
--type-add=make:is:Makefile.Release
--type-add=make:is:Makefile.Debug

That makes it easy to remove the ones you don't want.

n1vux commented 10 years ago

I don't see a need to complicate it with a regex. I'd just as soon make it... That makes it easy to remove the ones you don't want.

and shows good style to follow, to add more. I like it.

Should we add a doc patch suggesting adding any special makefile names in the users' toolchain into their ~/.ackrc in same style as seen in ConfigDefault.pm ?

mingwandroid commented 10 years ago

I agree that adding them literally is better than my regex. I had assumed that a subtractive filter was run at the end to globally remove things like *.bak, but if not then literal is best.

Do you want me to update the pull request? FWIW, if you committed it as per your suggestion I'm happy with that.

On Thu, Apr 24, 2014 at 5:52 PM, n1vux notifications@github.com wrote:

I don't see a need to complicate it with a regex. I'd just as soon make it...

That makes it easy to remove the ones you don't want.

and shows good style to follow, to add more. I like it.

Should we add a doc patch suggesting adding any special makefile names in the users' toolchain into their ~/.ackrc in same style as seen in ConfigDefault.pm ?

— Reply to this email directly or view it on GitHubhttps://github.com/petdance/ack2/pull/451#issuecomment-41304272 .

mingwandroid commented 10 years ago

Updated.

mingwandroid commented 10 years ago

Is there any update on this?

hoelzro commented 10 years ago

@mingwandroid Sorry for the delay, and thanks for the reminder! I don't see a problem with this, so I'm merging it.