~sircmpwn/aerc

Re: [PATCH] Add a 'folders-exclude' option

Details
Message ID
<C3TFLE4OQHOX.17ORBHPKCCZ5X@gux>
DKIM signature
pass
Download raw message
On Mon Jun 29, 2020 at 6:14 AM WAT, Reto wrote:
> Time appropriate greetings,

Hi!

> Here would be the function documentation, please do adapt it if you
> change the way it works.

Yup, I'll add that.

> > +	// 'folders' unset and 'folders-exclude' unset: show all
> > +	// 'folders' set and 'folders-exclude' unset: show 'folders'
> > +	// 'folders' unset and 'folders-exclude' set: show all except for 'folders-exclude'
> > +	// 'folders' set and 'folders-exclude' set: show 'folders' except for 'folders-exclude'
> That's maybe a tad excessive?
> I'm not sure that any comment is needed but if so it could be
> simplified to "use folders as an inclusion and folders-exclude as an
> exclusion filter"
> You really don't care if they are unset or set.

Makes sense. I was primarily using the comments to keep track of what
I was doing myself, but you're right, it's a bit much.

> In fact you could somewhat simplify the code and help the reader if
> you do something like:
> 
> ...
>
> That's just my personal preference though.
> If you really want you could even make it a method on dirlist
> directly.  I just thought that this method will only be used in this
> function so we might as well "inline" it...

I haven't actually used Go before (I mostly use C, C++, etc.), so I
just tried to use what was already there. The code can definitely be
improved - I'll incorporate some of your code.

> Feel free to pick whichever version you like better (yours or mine).
> However please do fix the docstring as well as reduce the comments
> if you don't mind.

Yup, see you in v2.
Export thread (mbox)