[aur-dev] [PATCH v3] Implement capability to pin comments above others

[Mark Weiman]

On Fri, 2015-12-11 at 20:42 +0100, Lukas Fleischer wrote:
> This version already looks pretty good! There are still some
> whitespace
> errors. You should see error messages like
> 
>     .git/rebase-apply/patch:212: trailing whitespace.
> 
> when you try to apply the patch yourself, e.g. by running
> 
>     $ git format-patch HEAD^
>     $ git reset --hard HEAD^
>     $ git am 0001-Implement-capability-to-pin-comments-above-
> others.patch
> 
> More comments below.
> 
> Why are these files executable?
> 

I have no idea how or why they are and it must have slipped past my
eyes when I checked thdiff --git a/schema/aur-schema.sql b/schema/aur-
schema.sql

> 
> Why is this different from the schema line above?
> 
>     ALTER TABLE PackageComments ADD COLUMN PinnedTS BIGINT UNSIGNED
> NOT NULL DEFAULT 0;
> 
> is what I would have expected here.
> 

I will change it to your suggestion.

> 
> Seems like pkgbase_pin_comment() and pkgbase_unpin_comment() are
> almost
> identical. Can we share most of the code?
> 

I thought about doing that, but I thought the names of the functions
were important for code readability. Perhaps adding an argument to
pkgbase_pin_comment() that does it and just have
pkgbase_unpin_comment() call the other function to change that
argument? Also the error messages have a slight difference in text that
also pushed me to making two separate functions.

> Also, we make sure that one cannot pin more than five comments but I
> would have expected the "Pin" icons to be hidden when the limit is
> reached. The current UI behavior is quite confusing.
> 

I'll change this.

> 
> This looks a bit weird. Wrong indentation?
> 

Gedit seems to enjoy changing my tab settings whenever I close it, I
will be double sure to check it when I use it in the future.

I'll do another sweep to see if there are more space indentations
rather than tab indentations. (UGH)

> 
> Why is this needed? Could you elaborate please?
> 

The reason I had to do this is the variable $base_id when opening the
file for a second time (to display all comments) was already set and
caused it to then be set to an empty string, so I added a check to see
if the variable was already set so if it is, it can avoid trying to set
it again removing the problem. I do not really understand why this
happens, but it does on my computer.

It seriously took me a couple of hours of head scratching to find that
problem and fix it because it really shouldn't have been an issue.

I also noticed that $base_id is set in both pkgbase_display_details()
and pkg_display_details(), should I just remove that assignment
altogether in pkg_comments.php?

> Thank you for all your hard work on implementing this!
> 
> Regards,
> Lukas

Mark Weiman