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