Re: [Semanticscuttle-devel] Added branch for configurable privacy ....

[Brett D. <bs...@fr...>]

Hello, Christian.

Thanks for the feedback/review. I will work to make corrections and
emend my changes over the next few days.

Given my as-yet limited experience with git, perhaps it would be best
for you to delete the branch.

Thanks.

Rgrds,
Brett

On Mon, 14 Mar 2011 08:33:56 +0100, Christian Weiske
<cw...@cw...> wrote:
> Hello Brett,
> 
> 
>> My apologies for the mix-up. I think I've corrected this by pushing to
>> the master at origin. However, now the configurable-privacy branch
>> shows two parents.
> No, not really. I had to do the diff manually since you also seemed to
> change the line breaks in all the files.
> 
> Did you notice you have a sc subdirectory? You also added files
> like Thumbs.db that should never be checked in the repository.
> 
> About the code itself:
> - You do not need to declare "$defaults = array()" in config.php.dist
>   since config.default.php is included before config.php - so
>   redeclaring the array would actually erase the default settings for
>   privacy.
> - Why do you suddenly display "public" for public bookmarks? Since
>   public is standard, there is no need to display the text. The
>   bookmark status is only shown for non-public bookmarks which helps to
>   unclutter the bookmark list display in my eyes.
> - Your unit test is flawed since it does not test the effects of a
>   privacy config setting at all - you set the variable and use the same
>   variable as parameter to the method you are calling.
>   The correct way is to set the privacy variable permanently in a
>   config and call the scripts you changed (i.e. posts_add.php, edit.php)
>   with a set of POST parameters via HTTP. You can then observe if the
>   added bookmarks have the configured status. Setting the varible
>   permanently is not easy here since you do a HTTP call, but I will add
>   that to the master branch quickly.
>   Always remember that for the unit tests to be sufficient they need to
>   fail when I remove or change a line in your SemanticScuttle source
>   code changes. Currently, I can revert the changes in www/bookmark.php
>   and all other files without the tests to fail.
> 
> Apart from that, the code seems to do what it should.
> 
> About the branch:
> Since the configurable-privacy branch is totally broken, we should just
> delete it and start afresh from master and apply your changes to it.
> Shall I do that for you?