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

[Christian W. <cw...@cw...>]

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?

-- 
Regards/Mit freundlichen Grüßen
Christian Weiske

-=≡ Geeking around in the name of science since 1982 ≡=-