knime-ip / knip-cellprofiler

KNIME Image Processing - CellProfiler Integration
3 stars 1 forks source link

Remove FIXME and TODOs #3

Closed dietzc closed 9 years ago

dietzc commented 9 years ago

We need to remove all FIXME and TODOs from the project before publishing

ctrueden commented 9 years ago

It's a lot! Across the SciJava orgs:

$ for f in imagej/* imglib/* knime/* scifio/* scijava/*
do
  cd $f &&
  count="$(git grep TODO | wc -l)" &&
  if [ "$count" -gt 0 ]
  then
    echo "$count\t$f"
  fi &&
  cd ../..
done
       1    imagej/IJ-guide
       2    imagej/ImageJA
       2    imagej/ij1-patcher
      94    imagej/ij1-tests
       2    imagej/imagej
     157    imagej/imagej-common
       4    imagej/imagej-launcher
      54    imagej/imagej-legacy
       8    imagej/imagej-omero
     151    imagej/imagej-ops
     118    imagej/imagej-plugins-commands
      18    imagej/imagej-plugins-tools
       1    imagej/imagej-ui-awt
      52    imagej/imagej-ui-swing
      19    imagej/imagej-updater
       2    imagej/imagej1
      28    imagej/matlab-plugins-in-imagej
       1    imagej/visad-plugin
      11    imagej/wiki.imagej.net
     137    imglib/imglib2
      38    imglib/imglib2-algorithm
       8    imglib/imglib2-algorithm-gpl
       9    imglib/imglib2-ij
       3    imglib/imglib2-realtransform
      22    imglib/imglib2-roi
     143    imglib/imglib2-script
      14    imglib/imglib2-tests
       1    imglib/imglib2-tutorials
       2    imglib/imglib2-ui
     336    knime/knip
       2    knime/knip-cellprofiler
       1    knime/knip-ilastik
      16    knime/knip-imagej2
       4    knime/knip-imagej2-integration
       2    knime/knip-imagej3dviewer
      71    knime/knip-imglib2-ops
       5    knime/knip-micromanager
       1    knime/knip-omero
       2    knime/knip-opencv
       4    knime/knip-scijava-bundles
      10    knime/knip-scripting
      51    knime/knip-suise
      37    knime/knip-tess4j
      12    knime/knip-vtk
       5    knime/util
      56    scifio/scifio
       3    scifio/scifio-bf-compat
       5    scifio/scifio-cli
       1    scifio/scifio-hdf5
       1    scifio/scifio-imageio
       1    scifio/scifio-jai-imageio
       1    scifio/scifio-lifesci
      10    scifio/scifio-ome-xml
       3    scifio/scifio-omero
       1    scifio/scifio.github.io
       5    scijava/minimaven
       2    scijava/native-lib-loader
      61    scijava/scijava-common
       2    scijava/scijava-plugins-commands
       4    scijava/scijava-ui-awt
       3    scijava/scijava-ui-pivot
      12    scijava/scijava-ui-swing
       5    scijava/scijava-ui-swt
     186    scijava/scijava.github.com
       1    scijava/scripting-jython

There are 336 TODOs in knime/knip alone.

FIXMEs are not so bad:

$ for f in imagej/* imglib/* knime/* scifio/* scijava/*
do
  cd $f &&
  count="$(git grep FIXME | wc -l)" &&
  if [ "$count" -gt 0 ]
  then
    echo "$count\t$f"
  fi &&
  cd ../..
done
       1    imagej/imagej
      12    imagej/imagej-common
      21    imagej/imagej-legacy
       1    imagej/imagej-omero
       6    imagej/imagej-ops
       8    imagej/imagej-plugins-commands
       3    imagej/imagej-plugins-tools
       7    imagej/imagej-ui-swing
       1    imagej/javafx-plugins
       9    imagej/wiki.imagej.net
       3    imglib/imglib2-algorithm
       3    imglib/imglib2-algorithm-fft
       1    imglib/imglib2-algorithm-gpl
       1    imglib/imglib2-tests
       4    knime/knip
       3    knime/knip-imglib2-ops
       1    knime/knip-opencv
       1    knime/knip-scripting
      28    scifio/scifio
       1    scifio/scifio-bf-compat
       1    scifio/scifio-hdf5
       1    scifio/scifio-ome-xml
       4    scifio/scifio-omero
       8    scijava/scijava-common
       1    scijava/scijava-ui-awt
       2    scijava/scijava-ui-pivot
       4    scijava/scijava-ui-swing
       3    scijava/scijava-ui-swt
       4    scijava/scripting-r

See also: https://github.com/chrishunt/git-pissed :grin:

dietzc commented 9 years ago

I will start with knip-cellprofiler and see where it goes or if I git pissed :sweat_smile:

dscho commented 9 years ago

cd $f && count="$(git grep TODO | wc -l)" && if [ "$count" -gt 0 ] then echo "$count\t$f" fi && cd ../..

A better way is to use the pattern (cd $f && ...). That way you do not have to hardcode that there are two directory levels.

ctrueden commented 9 years ago

@dscho As we discussed last time you made that point, the above approach is in response to another comment from you from earlier, that spawning a subshell each time has an overhead cost. Of course, I have not benchmarked this. But given that the command was a one-off which will not be encoded into a heavily used shell script, either way seems perfectly acceptable to me.

dscho commented 9 years ago

spawning a subshell each time has an overhead cost

It is negligible, except on Windows. But even there, your script's execution time will be dominated by far by the real operations.

However, speed considerations should never be a concern when correctness is affected. And cd abc && ... && cd .. just is not correct (imagine that some other process moved abc in the meantime, or that you change the script to change to abc/xyz).

Correctness trumps speed. Every single time.

dietzc commented 9 years ago

OT!

dietzc commented 9 years ago

at least here I did it