search

minishift / minishift-centos-iso

CentOS based ISO as an alternative for boot2docker ISO
GNU Lesser General Public License v3.0
40 stars 33 forks source link

Refactor phony targets #72

Closed coolbrg closed 7 years ago

coolbrg commented 7 years ago

Grouped phony targets. This will reduce lines and easy to maintain IMO.

@hferentschik I believe for such PR we need no related issue. It's related to refactoring.

hferentschik commented 7 years ago

Grouped phony targets. This will reduce lines

Number of lines is not all. Having the phony target next to the actual target makes it imo more obvious that a target is phony and secondly has a better chance to keeping in sync. As have have seen before, make does not care whether the names match or not. Having the phony target at the bottom will imo increase the risk that things get out of sync.

and easy to maintain IMO.

How so?

I believe for such PR we need no related issue. It's related to refactoring.

Why not. First of all you could explain in the issue why this is of any benefit and we could have a discussion. Secondly, being a refactoring does not mean no issue is needed.

From my side this whole pull request gets a -1.

hferentschik commented 7 years ago

@LalatenduMohanty @praveenkumar, not sure what you think. If you think this is better, go ahead, but please with an issue.

praveenkumar commented 7 years ago

-1 for the PR, having phony for each target make it more readable and maintainable also. Lot of projects are there which do exactly like we did.

LalatenduMohanty commented 7 years ago

@budhrg Having phony has some technical advantages and thats the reason it is widely used. Check https://www.gnu.org/software/make/manual/html_node/Phony-Targets.html

LalatenduMohanty commented 7 years ago

@budhrg going to close the PR as we think it is not required. Please reopen with an issue if you feel strongly that phony should not be present in Makefile.