[Octave-patch-tracker] [patch #7976] Make PPKG_ADD and PKG_DEL see both m-file and oct-file directories of a package (2nd post)

[Jordi Gutiérrez Hermoso]

Update of patch #7976 (project octave):

                  Status:                    None => In Progress            
             Assigned to:                    None => jordigh                

    _______________________________________________________

Follow-up Comment #1:

Your two csets depend on each other. Indeed, you'll see that the parent of
pkg-addpath-rmpath.changeset is addpath-rmpath.changeset. In situations like
these, it's best if you concatenate both into the same file. Here "hg export
-r -2:-1" would do it, assuming that -2 and -1 point to these two commits
(i.e. if they're the last two chronologically on your repository).

Your commit message for addpath-rmpath.changset has long lines. The first line
should be short, under 80 characters.

Typo:


(rmpath): fill vector of direcories and call load_path::rm_dirlst.


"direcories"?


+
+  for (std::vector<std::string>::const_iterator i = dirlst.begin ();
+       i != dirlst.end (); i++)
+    do_add (*i, at_end, warn);


We have some typedefs for those long iterator names, but why are you using
std::vector instead of std::list? You don't need random access on this
directory list. For the usual sizes of these lists, this difference in the
choice of container doesn't matter much, granted.


                  std::string dir = *p;
-
+                  dirlst.push_back (*p);
+
+                  //std::string dir = *p;
                   //dir = regexprep (dir_elts{j}, '//+', "/");
                   //dir = regexprep (dir, '/$', "");
 


Why are you leaving commented-out code? If the code needs to be deleted,
banish it to VCS history, not to a comment.

I'm not entirely happy with the whole architecture of this patch, but I'm not
entirely happy with the organisation of this whole load-path architecture
either. It feels to me like we're adding wrapper functions on top of wrapper
functions without fixing the actual underlying architectural problem.

Your second patch looks good. Small typo:


+  if (iscellstr (out_file))
+    if ((n = numel (out_file)) != numel (directory))
+      error ("number of output files must be one ore matching number of
directories");
+    endif


"ore"?

I recommend using Mercurial's histedit extension for polishing these two
patches. Briefly, after enabling the histedit extension, do


hg histedit -2


and this should open an editor where you can specify what to do to your last
two revisions. Select "edit" for both of them, close the editor, and edit each
revision in turn, doing 


hg histedit --continue


when you're done with each one.

    _______________________________________________________

Reply to this item at:

  <http://savannah.gnu.org/patch/?7976>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.gnu.org/