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
