On Mon, Mar 09, 2015 at 02:59:54PM -0400, Jason Pleau wrote: > Updated patch that fixes an error if hidden_files is empty
Hey Jason, thanks a lot for your patch! It looks great in general. I just have a few minor changes to request, if you don't mind. See below for details. > Right now the .pc/ exclusion in directory listings is hardcoded. This > new hidden_files configuration allows us to be more flexible in what is > shown or hidden in those listings. > > */.pc/ is added as a default value for this configuration setting. This is one concern I have. You've modified the sample config file, but not the default configuration stored in mainlib.py. I haven't tested that, but I suspect that for that reason upgrading a currently deployed Debsources instance to a version that include your patch will trigger failures at runtime, if config.local.ini isn't updated. Can you confirm that? If so, I'd very much prefer changing DEFAULT_CONFIG in mainlib.py to include */.pc/ as default setting to 1) avoid breakages, and 2) retain the current behavior. > diff --git a/debsources/app/templates/source_folder.html > b/debsources/app/templates/source_folder.html Template and view changes look good, and I daresay that models look nicer with your changes :) > +# space-separated list of files or directories patterns to hide in > +# directory listings > +hidden_files: */.pc/ It would be nice to document in the comment here (for lack of a better place...) the intended semantics of hidden_files, i.e.: match on the full path, relative to which dir, and maybe the fact that fnmatch() is involved, for reference. There are also a couple of minor flake8 issues in your patch (which I can fix upon patch import, if needed). And from your other email: > This also allow us to add a toggle button (show / hide hidden files) in > directory listings eventually. I could do that in a separate commit in > this bug if you'd like. Yes please, that would be awesome! :) Cheers. -- Stefano Zacchiroli . . . . . . . z...@upsilon.cc . . . . o . . . o . o Maître de conférences . . . . . http://upsilon.cc/zack . . . o . . . o o Former Debian Project Leader . . @zack on identi.ca . . o o o . . . o . « the first rule of tautology club is the first rule of tautology club » -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
