Bug#762944: debsources: make .pc/ exclusion a configuration parameter
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 . . . . . . . zack@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 »
Reply to: