Add some comments to several functions

[ismaelgv]

As we discussed in #443, I've added some comments to the source code in places where it was hard to understand or confusing to me. Please check them to be sure I understood correctly before merging this to pacaur50.

In addition, I've got observations on some of that functions:

IgnoreChecks() IgnoreDepsChecks()

These two were particularly hard to understand even when their functionality is not that complex. I think that there is much repeated code here that could be converted to small functions, for instance, checking the ignore groups.

ProviderChecks() ConflictChecks()

As you said, these functions are a bit convoluted, specially ProviderChecks(). However, their functionality is not simple and the problem they solve is not trivial either. Also, I think they are at least well documented. Anyways, i think that some part of them can be splitted into simple functions.

ReinstallChecks()

I don't clearly get what are the initial checks to skip the packages:

[[ ! $foreign ]] && [[ ! " ${aurpkgs[@]} " =~ " ${depsAname[$i]} " || " ${aurconflictingpkgs[@]} " =~ " ${depsAname[$i]} " ]] && continue
[[ -z "${depsQver[$i]}" || "${depsQver[$i]}" = '#' || $(vercmp "${depsAver[$i]}" "${depsQver[$i]}") -gt 0 ]] && continue
[[ ! $installpkg && ! " ${aurdepspkgs[@]} " =~ " ${depsAname[$i]} " ]] && continue

OutofdateChecks() OrphanChecks() GetBuiltPkg() CleanCache() Proceed()

These functions are nice and a good example to follow when other functions are split. They do one thing at the time on a few lines.

Prompt()

Binary size computation could be extracted from here and put into an utils simple function.

EditPkgs()

Here, there are many nested loops and they are a bit hard to follow. Some functions could be created to handle this. I've noticed that options taken from global configuration and use as conditional make the code a lot of harder to read (p.e. $noconfirm).

MakePkgs()

I'm positive that a substantial part of this function can be extracted to new functions. A clear example is the fragment of code where is checked the integrity. I am also wondering if the new orphan checks couldn't use OrphanCheck() in some way. As you suggested, some parts of the code should be rewritten using a functional approach and this is a clear target to start with.

[rmarquis]

As we discussed in #443, I've added some comments to the source code in places where it was hard to understand or confusing to me. Please check them to be sure I understood correctly before merging this to pacaur50.

First of all, thanks to you for taking the time to look at the code. Very much appreciated.

These two were particularly hard to understand even when their functionality is not that complex. I think that there is much repeated code here that could be converted to small functions, for instance, checking the ignore groups.

Yes, they are most the same function for different set of packages (AUR targets and their AUR deps). It might even be possible to make them entirely functional and have only one function for both use. I remember I had quite troubles to make them working right, it's very possible I overcomplexified them unecessarily. Complete review and rewrite is in order I guess.

As you said, these functions are a bit convoluted, specially ProviderChecks(). However, their functionality is not simple and the problem they solve is not trivial either. Also, I think they are at least well documented. Anyways, i think that some part of them can be splitted into simple functions.

Yes. Using pacsift from pacutils will also simplify some of that code.

I don't clearly get what are the initial checks to skip the packages:

This is all done to avoid false positives in all the edges cases, for example 1/ with --needed or conflicting packages, 2/ packages provided by others or with weird versioning scheme, and 3/ with -Sw.

Some of that code is 3 or 4 years old, and I'm not excluding that some of that code is obsolete today. However, the main issue I see is that ReinstallCheck() is tightly coupled with the previous code instead of being independent.

These functions are nice and a good example to follow when other functions are split. They do one thing at the time on a few lines.

Good. Incidentally, these are part of code that can be considered "independent" and not tightly coupled.

Binary size computation could be extracted from here and put into an utils simple function.

Absolutely.

Here, there are many nested loops and they are a bit hard to follow. Some functions could be created to handle this. I've noticed that options taken from global configuration and use as conditional make the code a lot of harder to read (p.e. $noconfirm).

I'd even say the level of nested conditional code is absolutely insane. There is undoubtedly something broken in the mind of the guy that coded that. To his defense, I'd say this is mostly the result of incremental changes without making the required refactoring :)

I'm positive that a substantial part of this function can be extracted to new functions. A clear example is the fragment of code where is checked the integrity. I am also wondering if the new orphan checks couldn't use OrphanCheck() in some way.

You might confuse OrphanCheck() and locally orphan packages. The former warns about packages marked as orphan in the AUR, while the latter is about packages that not explictly installed but not needed by any other package. Sadly, the same "orphan" term is used for both in the Arch community, so this is a bit confusing.

The good thing about MakePkgs() is that it is pretty much independent from the rest of the code. It takes the ordered package names to build as input, and does the rest by itself. Of course, some part can be decouped in subfunctions to reduce duplication of code.

As you suggested, some parts of the code should be rewritten using a functional approach and this is a clear target to start with.

Yes. The past few days I've had a closer look at makepkg and aurutils code, and pacaur could profit of a similar functional approach in many way. It might not be possible to obtain something as clean (after all bash sucks when working with arrays), but there is a large margin for improvement here.

Again, thanks a lot for your review.

[ismaelgv]

Yes, they are most the same function for different set of packages (AUR targets and their AUR deps). It might even be possible to make them entirely functional and have only one function for both use. I remember I had quite troubles to make them working right, it's very possible I overcomplexified them unecessarily. Complete review and rewrite is in order I guess.

We could open an issue to track it down and make some code prototypes. We could even test it on this branch.

Yes. Using pacsift from pacutils will also simplify some of that code.

It sounds like a good idea.

This is all done to avoid false positives in all the edges cases, for example 1/ with --needed or conflicting packages, 2/ packages provided by others or with weird versioning scheme, and 3/ with -Sw.

I will add some comments to this part to clarify it until is rewritten/reviewed.

Binary size computation could be extracted from here and put into an utils simple function.

I think this can be easily extracted without much trouble.

I'd even say the level of nested conditional code is absolutely insane. There is undoubtedly something broken in the mind of the guy that coded that. To his defense, I'd say this is mostly the result of incremental changes without making the required refactoring :)

You made me laugh there. 😆

You might confuse OrphanCheck() and locally orphan packages. The former warns about packages marked as orphan in the AUR, while the latter is about packages that not explictly installed but not needed by any other package. Sadly, the same "orphan" term is used for both in the Arch community, so this is a bit confusing.

I did not think about that. Well, I suppose it is fine then.

The good thing about MakePkgs() is that it is pretty much independent from the rest of the code. It takes the ordered package names to build as input, and does the rest by itself. Of course, some part can be decouped in subfunctions to reduce duplication of code.

Nice, that will ease further refactoring. Anyways, I will start with other simpler parts of the code since that is a pretty critical function that I do not want to break.

Yes. The past few days I've had a closer look at makepkg and aurutils code, and pacaur could profit of a similar functional approach in many way. It might not be possible to obtain something as clean (after all bash sucks when working with arrays), but there is a large margin for improvement here.

I will also take another look to those programs, they are a really good code reference.

Again, thanks a lot for your review.

Thanks to you for answering that fast! You really encourage me to contribute and I am happy to help here. 😄

I will try to make some changes on the code comments that you reviewed before as soon as possible. I am going to be offline/busy the next month but I think that I can complete this PR and maybe open one or two more with some simple stuff this week.

[ismaelgv]

I've made some changes following your comments. In addition, I have renamed all relevant binary dependency to repo dependency in the comments to keep the consistency.

[ismaelgv]

If these comments are OK I can squash and merge this PR to pacaur5.

[rmarquis]

Sorry, I forgot about it. Minor changes only, otherwise ok!